IBX-8980: Fixed getGroupedFields method implementation #77
Conversation
|
| $groupedFields = []; | ||
|
|
||
| foreach ($fieldsDataForm as $fieldForm) { | ||
| /** @var \Ibexa\Contracts\ContentForms\Data\Content\FieldData $fieldData */ | ||
| $fieldData = $fieldForm->getViewData(); | ||
| $fieldGroupIdentifier = $this->fieldsGroupsList->getFieldGroup($fieldData->fieldDefinition); | ||
| $fieldGroupName = $fieldsGroups[$fieldGroupIdentifier] ?? $this->fieldsGroupsList->getDefaultGroup(); |
There was a problem hiding this comment.
Even tho it seems like a bug, not sure we can fix it that way due to bc policy. Otoh, this seems so internal and if only used by that one place (is it tho?) it should be acceptable.
@ibexa/php-dev
There was a problem hiding this comment.
It seems that $fieldGroupName which is a key in the returned array is used in other places (admin-ui, product-catalog).
- https://github.com/ibexa/admin-ui/blob/4.6/src/bundle/Resources/views/themes/admin/content/edit_base.html.twig#L52
- https://github.com/ibexa/product-catalog/blob/4.6/src/bundle/Resources/views/themes/admin/product_catalog/product/create.html.twig#L77
Instead of the human-readable name identifier will be displayed
|
Alright, I will try then implementing a new provider and update the templates in |
Sztig
force-pushed
the
IBX-8980-grouped-fields-fix
branch
from
81aef34 to
d53ab22
Compare
| /** @var \Ibexa\Core\Helper\FieldsGroups\FieldsGroupsList */ | ||
| private $fieldsGroupsList; |
There was a problem hiding this comment.
Prefer typed properties over annotations, when dealing with PHP 7.4+ and no BC.
| /** @var \Ibexa\Core\Helper\FieldsGroups\FieldsGroupsList */ | |
| private $fieldsGroupsList; | |
| private FieldsGroupsList $fieldsGroupsList; |
| ]); | ||
|
|
||
| $subject = new GroupedContentFormFieldsProvider($fieldsGroupsListMock); | ||
| $subject = new NonLocalizedGroupedContentFormFieldsProvider($fieldsGroupsListMock); |
There was a problem hiding this comment.
This - given it's a separate class - should be in it's own test case/class. Currently you're replacing a test for existing class with this one.
There was a problem hiding this comment.
👍 I will make a new class for tests for the new provider and revert the old test as they were.
| @@ -13,6 +13,9 @@ | |||
| use JMS\TranslationBundle\Model\Message; | |||
| use JMS\TranslationBundle\Translation\TranslationContainerInterface; | |||
|
|
|||
| /** | |||
| * @deprecated since 4.6.17 this class is deprecated. Please use NonLocalizedGroupedContentFormFieldsProvider instead | |||
There was a problem hiding this comment.
FQCN could be used here
|
Related PRs:
Description:
Based on the interface of
GroupedContentFormFieldsProviderthe methodgetGroupedFieldsis implemented incorrectly. The description of the interface statesArray of fieldGroupIdentifier grouped by fieldGroupNameshould be returned. Right now it doesn't return thefieldGroupIdentifierbut a translation of said identifier.For the example the previously retrieved key would be
Basic Information, after the fix it's going to bebasic_information