-
Notifications
You must be signed in to change notification settings - Fork 18
IBX-8753: Improved ModifyFieldDefinitions logic #1614
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 4.6
Are you sure you want to change the base?
Conversation
…yFieldDefinitions logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 but we need to improve code readability. I think we can start by splitting onPreSetData logic into several small private methods: https://gist.github.com/adamwojs/e1934ded9bceb3eb92d1f7b8f5c23fdc
… readability and encapsulate logic with helper methods
string $fieldTypeIdentifier, | ||
array $modifiedOptions, | ||
$fieldIdentifiers = [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are there two parameters that are named in the exact same way ($fieldIdentifier
and $fieldIdentifiers
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There’s no $fieldIdentifier parameter — only $fieldTypeIdentifier (string) and $fieldIdentifiers (array).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about deprecating passing string
into it, and just keep array for 5.0?
@ibexa/php-dev
…dency and updated related logic
…rface and updated related logic
|
Description:
This PR extends the ModifyFieldDefinitionFieldsSubscriber to support the following:
For QA:
Documentation: