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

[Listing] Question mark and colon in condition in addConditionParam() are always interpreted as prepated statement parameters #11857

Merged
merged 6 commits into from
Jun 7, 2022

Conversation

BlackbitDevs
Copy link
Contributor

@BlackbitDevs BlackbitDevs commented Apr 6, 2022

Step to reproduce:

$listing = new Listing();
$listing->addConditionParam('name="Brand: Test product"');
$listing->load();

This will result in an SQL error / PDO exception:
SQLSTATE[HY093]: Invalid parameter number: number of bound variables does not match number of tokens

The reason is that in

$ignore = true;
if (strpos($key, '?') !== false || strpos($key, ':') !== false) {
$ignore = false;
}
$this->conditionParams[$key] = [
'value' => $value,
'concatenator' => $concatenator,
'ignore-value' => $ignore, // If there is not a placeholder, ignore value!
];
it is always assumed that when there is a : or a ? in the SQL condition that this is a query parameter.
So the same problem appears when

$listing->addConditionParam('name="This is an object name?"');

Above are simplified examples, of course normally you would use

$listing->addConditionParam('name=?', ['Brand: Test product']);

but sometimes this is not easily possible.

An alternative solution would be to add Listing::addCondition() to add conditions without parameters.
Imho, it is not a BC break, so if you want I could also rebase to 10.3 branch.

@jdreesen
Copy link
Contributor

jdreesen commented Apr 6, 2022

This contains a commit from PR #11817

@BlackbitDevs BlackbitDevs changed the title [Listing] Question mark and colon in condition are always interpreted as prepated statement parameters [Listing] Question mark and colon in condition in addConditionParam() are always interpreted as prepated statement parameters Apr 7, 2022
@brusch brusch added the Bug label Apr 7, 2022
@aryaantony92
Copy link
Contributor

aryaantony92 commented Apr 19, 2022

@BlackbitNeueMedien Since this is a bug, could you please rebase this to 10.3 branch ?

@BlackbitDevs BlackbitDevs changed the base branch from 10.x to 10.3 April 21, 2022 06:49
@BlackbitDevs BlackbitDevs changed the base branch from 10.3 to 10.x April 21, 2022 06:50
@BlackbitDevs BlackbitDevs changed the base branch from 10.x to 10.3 April 21, 2022 06:56
@BlackbitDevs
Copy link
Contributor Author

@aryaantony92 Is now for 10.3 branch.

@aryaantony92
Copy link
Contributor

@BlackbitNeueMedien Thanks for rebasing.

But even after applying the patch/commit, when I try to do

$listing->addConditionParam('name="Brand: Test product"'); Or
$listing->addConditionParam('name="This is an object name?"');

I can still see the error message

"SQLSTATE[HY093]: Invalid parameter number: number of bound variables does not match number of tokens"

@BlackbitDevs
Copy link
Contributor Author

BlackbitDevs commented Apr 21, 2022

Sorry, regex was not correct, now it should work, even for

$listing->addConditionParam('name="Brand: " and o_path=? and o_key="test?"', ['/']);

@brusch
Copy link
Member

brusch commented Apr 26, 2022

I'd say this is quite a critical thing that should be covered by a test, could you please add one for all the possible cases? Thanks in advance!

@brusch brusch changed the base branch from 10.3 to 10.4 April 27, 2022 15:41
@brusch
Copy link
Member

brusch commented May 2, 2022

@BlackbitNeueMedien btw. writing tests is now super easy, see: #11872 and https://pimcore.com/docs/pimcore/current/Development_Documentation/Development_Tools_and_Details/Testing/Core_Tests.html

Thanks in advance!

@brusch
Copy link
Member

brusch commented May 9, 2022

@BlackbitNeueMedien 🏓 one more try 😉

@BlackbitDevs
Copy link
Contributor Author

Yep, did not forget it. Currently implementing #10924, try to come back to this PR here for the tests (or perhaps you would be so kind and create this?)

@brusch
Copy link
Member

brusch commented May 9, 2022

@BlackbitNeueMedien ah cool 👍 then we wait until you're finished with the other topic 😉 Thanks in advance!

@BlackbitDevs
Copy link
Contributor Author

@brusch I added the tests.

@brusch brusch added this to the 10.4.3 milestone Jun 7, 2022
@brusch brusch merged commit 17af0b5 into pimcore:10.4 Jun 7, 2022
@brusch
Copy link
Member

brusch commented Jun 7, 2022

Thank you very much!

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

4 participants