Skip to content
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

IBX-8980: Fixed getGroupedFields method implementation #77

Open
wants to merge 2 commits into
base: 4.6
Choose a base branch
from

Conversation

Sztig
Copy link

@Sztig Sztig commented Nov 21, 2024

🎫 Issue IBX-8980

Related PRs:

Description:

Based on the interface of GroupedContentFormFieldsProvider the method getGroupedFields is implemented incorrectly. The description of the interface states Array of fieldGroupIdentifier grouped by fieldGroupName should be returned. Right now it doesn't return the fieldGroupIdentifier but a translation of said identifier.

For the example the previously retrieved key would be Basic Information, after the fix it's going to be basic_information

@Sztig Sztig requested a review from a team November 21, 2024 11:07
Copy link

sonarcloud bot commented Nov 21, 2024

$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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ViniTou ViniTou requested a review from a team December 9, 2024 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants