Skip to content
This repository has been archived by the owner on Aug 1, 2024. It is now read-only.

fix: allow array values for expression parameters #366

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

spackmat
Copy link
Contributor

Fixes the problem described in #364 where array values for parameters of filter expressions are misinterpreted as [value, type] pairs, throwing strange errors within Doctrine.

When the value (with an optional type) is wrapped inside a ExpressionParameterValue value object, the DoctrineApplyFilterListener detects and used that explicit declaration. When the value is an array, it is checked if it contains a valid DBAL-type string on the second element, before it is assumed that it contains an implicit type decleration in that old array form. That branch can be deprecated in favor of the explicit declaration using the new ExpressionParameterValue class.

Fixes the problem described in lexik#364 where array values for parameters of filter expressions are misinterpreted as [value, type] pairs, throwing strange errors within Doctrine.

When the value (with an optional type) is wrapped inside a `ExpressionParameterValue` value object, the `DoctrineApplyFilterListener` detects and used that explicit declaration. When the value is an array, it is checked if it contains a valid DBAL-type string on the second element, before it is assumed that it contains an implicit type decleration in that old array form. That branch can be deprecated in favor of the explicit declaration using the new `ExpressionParameterValue` class.
In 5b99c11 the tests failed do to not updated assertions.
@gilles-g
Copy link
Member

Hi @spackmat,

Thx for the PR.

I need to found a moment to check your PR, did you add tests? Because you have just change an existant test 🤔

@spackmat
Copy link
Contributor Author

spackmat commented Jan 31, 2023

did you add tests? Because you have just change an existant test 🤔

I did not add tests for that case, because the fixtures don't contain a relation, on which an in() expression could be tested. So I just changed the few tests that test createCondition() at all so they use query parameters instead of embedding the value-strings inside the expression. They all use my proposed ExpressionParameterValue class, which in that case is not really necessary, but at least creates a litte test coverage for that class.

This changes the AbstractDoctrine- and DoctrineORMSubscriber to use the new `ExpressionParameterValue`
@spackmat
Copy link
Contributor Author

spackmat commented Feb 1, 2023

I also removed the internal usages of the old parameter-array-format and changed them to use the ExpressionParameterValue, so now I could also trigger a deprecation message, when that parameter-array-format is detected. Should I implement that by?

@trigger_error('Setting Expression parameters with array format `[$value, $type]` is deprecated, use `new ExpressionParameterValue($value, $type)` instead.', \E_USER_DEPRECATED);

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

Successfully merging this pull request may close these issues.

2 participants