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

Fixes parenthesis when \n is involved #8453

Merged
merged 3 commits into from Feb 22, 2021
Merged

Conversation

Warxcell
Copy link
Contributor

@Warxcell Warxcell commented Feb 5, 2021

JSON_CONTAINS(p.state, :positive_value, :ordered_state) = :positive_value 
OR 
JSON_CONTAINS(p.state, :positive_value, :completed_state) = :positive_value)

when we have OR like that (without space before OR) - it doesn't add parenthesis.

@greg0ire
Copy link
Member

I guess there is the same issue with tabulations, right?

@Warxcell
Copy link
Contributor Author

Warxcell commented Feb 22, 2021

I guess there is the same issue with tabulations, right?

I don't know. Who uses tabs on queries anyways?

EDIT: Just tried it out, my IDE converts automatically tabs to multiple spaces..

Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

Yeah, but maybe str_replace is faster. If we will use RegEx, I can just use preg_match and not replace anything.

Probably, but here I don't think we are in the hot path: we are in dql parsing which:

  • is cached
  • does not happen a lot (a few queries vs other code that applies on every row of the result set)

@Warxcell
Copy link
Contributor Author

Warxcell commented Feb 22, 2021

  • is cached
  • does not happen a lot (a few queries vs other code that applies on every row of the result set)

I think it's not the actual flow.

This is QueryBuilder, which builds DQL which is then parsed and after that cached.
So next time we have again QueryBuilder, which builds DQL, (skips parsing, because it's cached), then it's returned by cache.

But yeah, 30-40 preg_matches more (how many QB can have a single page?) - won't be anything significant.

@greg0ire
Copy link
Member

Hmmm yes I think you're right, it's probably executed every time indeed 👍
Anyway, not significant at all IMO.

Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

The pattern is not a expensive one. I think this should be fine.

@greg0ire greg0ire added the Bug label Feb 22, 2021
@greg0ire greg0ire merged commit a70c73a into doctrine:2.8.x Feb 22, 2021
@greg0ire
Copy link
Member

Thanks @Warxcell ! I squashed your commits because the first one and the last one really should, and although you made an argument for squashing in another PR, in this case it's unlikely to apply.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants