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

Reverse search and replace arrays in str_replace #3137

Merged
merged 1 commit into from
Mar 23, 2022

Conversation

HeinDR
Copy link
Contributor

@HeinDR HeinDR commented Mar 23, 2022

The parameters in the search array are appended with an index number. This causes problems when the elements in the array exceed 10 because the first element will find a part in the 10th element (e.g. content.topics = :topics_1 and content.topics = :topics_10). The resulting query will be corrupted because the 0 (in case of the 10th element) will not be replaced and left dangling in the string.
This change fixes this by reversing the array's, so the largest string will be looked up and replaced first.

The parameters in the search array are appended with an index number. This causes problems when the elements in the array exceed 10 because the first element will find a part in the 10th element (e.g. `content.topics = :topics_1` and `content.topics = :topics_10`). The resulting query will be corrupted because the 0  (in case of the 10th element) will not be replaced and left dangling in the string.
This change fixes this by reversing the array's, so the largest string will be looked up and replaced first.
@bobdenotter
Copy link
Member

Hi @HeinDR , 👋

Looks like you bumped into a legitimate usecase, but I'm not a huge fan of this particular fix. It feels "off".

Would it be a solution to use something like this, on line 583 of SelectQuery.php, instead? That way, they'd get params like topics_005.

$nameParam = sprintf('field_%03d', $filter->getKey());

@HeinDR
Copy link
Contributor Author

HeinDR commented Mar 23, 2022

Hi @bobdenotter,

It could be a solution, but then the issue will occur again with more than 10000 fields. I agree it's a highly unlikely scenario, but still. Line 583 would also not be the place to fix this because getRegularFieldWhereExpression is called on line 572 and the number index on the field is set by the QueryParameterParser if I'm not mistaken.
I agree that my solution feels a bit hackish, but as long as it is guaranteed that the index numbers are sequential, and I think it is, it is a solid solution I think.
Fixing this in the QueryParameterParser would probably have more impact.

Copy link
Member

@bobdenotter bobdenotter left a comment

Choose a reason for hiding this comment

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

Fair enough! In that case, let's get this merged! 🚂

Thanks, @HeinDR!

@bobdenotter bobdenotter merged commit dce0790 into bolt:master Mar 23, 2022
@bobdenotter bobdenotter changed the title Reverse search and replace array's in str_replace Reverse search and replace arrays in str_replace Mar 31, 2022
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