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-8897: Fixed incorrect extrapolation of parentheses in the name schema pattern #449

Open
wants to merge 1 commit into
base: 4.6
Choose a base branch
from

Conversation

Sztig
Copy link

@Sztig Sztig commented Nov 4, 2024

🎫 Issue IBX-8897

Description:

Fixing the regression introduced in #242. Right now, for example, if we us this name schema <title|(<field1> - <field2>)> (foobar) the foobar would be parsed to (EZMETAGROUP_0) regardless of the previously parsed fields. Grouping functionality with () has no use if the content inside this group has no <fields>.

@Sztig Sztig requested a review from a team November 4, 2024 15:19
Copy link

sonarcloud bot commented Nov 4, 2024

Comment on lines +254 to +257
// Skip the group if it has no fields to parse
if (!preg_match('/<.*>/', $group)) {
continue;
}
Copy link
Member

Choose a reason for hiding this comment

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

After looking into this we could probably do more complex regex in L248:

// match groups "( )" containing fields "< >" - curly brackets without any fields are not a group, but a literal string
$foundGroups = preg_match_all('/\((.*<.+>.*)\)/U', $nameSchema, $groupArray);

But as we discussed internally, I'm not sure which solution would be better for @ibexa/php-dev. The current one is maybe more readable.

POV ping @ibexa/php-dev

Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to proceed with the above, then tests related to this functionality should be expanded (which would be welcome by itself anyway, by the way).

This would ensure that any changes to the name schema processing, including regexp changes to improve performance, are not breaking anything - especially not any nested patterns.

Copy link
Author

Choose a reason for hiding this comment

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

I've already modified a test to cover the "new" part of this functionality, is it sufficient of should i cover more cases like so?

@alongosz alongosz requested a review from a team November 12, 2024 16:03
@alongosz alongosz changed the title IBX-8897: Parentheses in the name pattern field causes it to be displayed incorrectly FIX IBX-8897: Fixed incorrect extrapolation of parentheses in the name schema pattern Nov 12, 2024
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.

5 participants