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

Added warning about query cache in relation to parameters #10276

Merged
merged 4 commits into from
Dec 24, 2022
Merged

Added warning about query cache in relation to parameters #10276

merged 4 commits into from
Dec 24, 2022

Conversation

antman3351
Copy link
Contributor

Hi,
It's probably not worded the best but I felt the need to add a warning in the docs about the query cache to hopefully help others not make the same mistake I did.

Although the docs do state: "Parameters for the query should be set on the filter object by SQLFilter#setParameter(). Only parameters set via this function can be used in filters" I think it's not quite clear why and if developing with the query cache off can lead to hard to find bugs later on.

@derrabus
Copy link
Member

This warning indeed feels a bit redundant, given that the paragraph above it already explains how parameters should be dealt with. Maybe we can rephrase the existing paragraph instead of adding that warning?

@antman3351
Copy link
Contributor Author

Hi @derrabus, Instead of the paragraph "Parameters for the query should be set..."
how about ?:

"For the filter to correctly function the following rules must be followed, failure to do so will lead to unexpected results from the query cache.

  1. Parameters for the query should be set on the filter object by SQLFilter#setParameter() before the filter is used by the ORM ( i.e. do not set parameters inside SQLFilter#addFilterConstraint() function ).
  2. The filter must be detrimistic. Don't cahange the values base on external inputs.

The SQLFilter#getParameter() function takes care of the proper quoting of parameters. "

@derrabus
Copy link
Member

Yeah, something like that. Can you adjust the PR?

docs/en/reference/filters.rst Outdated Show resolved Hide resolved
docs/en/reference/filters.rst Outdated Show resolved Hide resolved
@derrabus derrabus changed the base branch from 2.13.x to 2.14.x December 20, 2022 08:19
@SenseException SenseException merged commit 0aa45dd into doctrine:2.14.x Dec 24, 2022
@SenseException
Copy link
Member

Thank you @antman3351

@derrabus derrabus added this to the 2.14.1 milestone Dec 28, 2022
derrabus added a commit to derrabus/orm that referenced this pull request Dec 28, 2022
* 2.15.x:
  Support of NOT expression from doctrine/collections ^2.1 (doctrine#10234)
  Fix Psalm errors with Collection 2.1.2 (doctrine#10343)
  Added warning about query cache in relation to parameters (doctrine#10276)
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.

3 participants