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

#1168 Add support for array parameters on the SQLFilter #5571

Closed
wants to merge 1 commit into from

Conversation

mnogales
Copy link
Contributor

No description provided.

@holtkamp
Copy link
Contributor

This resolves #2624

@billschaller
Copy link
Member

This adds a few new ways in which the SQLFilter can fail horribly. If objects are used as parameters, and the objects are later changed without calling setParameter again, this would break query caching in hard to debug ways.

@holtkamp
Copy link
Contributor

@zeroedin-bill, and that would not be possible when only one parameter was used instead of an array of parameters?

@@ -72,7 +73,11 @@
final public function setParameter($name, $value, $type = null)
{
if (null === $type) {
$type = ParameterTypeInferer::inferType($value);
if (is_array($type)) {
Copy link
Member

Choose a reason for hiding this comment

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

Block inside this conditional is not reachable.

Copy link
Member

Choose a reason for hiding this comment

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

Please revert this change to array inferring, the ParameterTypeinferer is already responsible for that itself.

Then this PR is good to go for 2.9.x in my POV.

@billschaller
Copy link
Member

@holtkamp Mostly referring to the Traversable possibility. If you pass an array, and the original array changes after it is passed, you would not affect the filter in any way, because the filter would hold a copy. If you pass a Traversable object, and then change that object after it was passed to the filter, the filter's state could change without setting the dirty flag.

I don't think anyone should be passing an object to a SQLfilter period, for that reason. The SQLFilter interface and phpdoc kind of hints that anything passed should be primitive, but it's not specified explicitly. The documentation should be made more explicit about what not to do with a SQLFilter, really, but that's not really in the scope of this PR.

This needs more tests IMO, especially around the $type param, and with all three types of allowable arrays.

@beberlei
Copy link
Member

beberlei commented Dec 6, 2020

Closing in favor of #8375

@beberlei beberlei closed this Dec 6, 2020
@beberlei beberlei removed this from the 2.9.0 milestone Dec 6, 2020
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