Ticket #1132 (closed defect: fixed)

Opened 13 months ago

Last modified 9 months ago

dqlSelectHook problem when adding the params in the execute part

Reported by: tuct Owned by: romanb
Priority: major Milestone:
Component: Query/Hydration Version: 1.0.3
Severity: Keywords:
Cc: Has Test: yes
Status: Commit Approved Has Patch: yes

Description

i got some problems with the dql Hooks again :) when using the execute() part of a query to parse the params, the order of the params for the query is wrong:

username = "test" -> SELECT u.* FROM user u WHERE u.username = ? AND u.deleted = ? - (0,'test')

maybe there is a better fix but i attached what worked for me.

i had to change some CacheQueryTests? but i think they a more adequate now, i don't think that it is normal to query the same query twice but using a new QueryObject? is more natural

Attachments

SoftDeleteTestCase.php (1.3 KB) - added by tuct 13 months ago.
tests/SoftDeleteTestCase.php
CacheTestCase.patch (1.1 KB) - added by tuct 13 months ago.
tests/Query/CacheTestCase.php
Abstract.patch (0.5 KB) - added by tuct 13 months ago.
lib/Doctrine/Query/Abstract.php [new One]

Change History

Changed 13 months ago by tuct

tests/SoftDeleteTestCase.php

Changed 13 months ago by tuct

tests/Query/CacheTestCase.php

Changed 13 months ago by jwage

  • milestone changed from 0.11.0 to 0.12.0

I added test coverage in r4518

We will introduce a fix for this in 0.12 as the fix will potentially break BC. You can work around this issue by adding all your params to the addWhere()/where() methods and avoid using execute() to pass params.

Changed 13 months ago by tuct

i know that i can work around this by not using the execute part for the params, the problem is that there are some part of doctrine that does so(find, findOne are already fixed for example) i came a cross this issue while using $object->RelatedObjects? (for getting the related objects with the Alias of the Relation,) with SoftDelete? active for the related class in this case, depending on the relationtype, a version of fetchRelatedFor() and getRelationDql() are called (Doctrine_Relation_Association in my case)

here the params are added to the query in the execute part and the query will be executed with the wrong order of params (very bad, can lead to really ugly bugs)

i can't see a fast way to change this or to not use the "auto" functions for fetching related objects

Changed 13 months ago by tuct

ok this is really nice :) i think i found a fix with won't break any BC and is just a copy and paste i updated Abstract.patch what do you think?

(this only solves the 2 new tests and break no others)

Changed 13 months ago by tuct

lib/Doctrine/Query/Abstract.php [new One]

Changed 13 months ago by tuct

  • has_patch set
  • has_test set

Changed 12 months ago by jwage

  • mystatus changed from Pending Core Response to Commit Approved

That is indeed what we were planning on doing, but we have to do it in 0.12 because it could break BC for some people. We will have to give it proper announcement so people can expect the behavior change of parameter precedence.

Changed 12 months ago by tuct

i think i would be good to apply this batch sooner then later, cause it potentially breaks BC, so people get used to it sooner what do you think?

Changed 11 months ago by guilhermeblanco

  • status changed from new to closed
  • resolution set to fixed

In r4733 fixed #1132. Thanks for the patch & tests!

Changed 10 months ago by anonymous

  • milestone New deleted

Milestone New deleted

Changed 9 months ago by tuct

  • status changed from closed to reopened
  • version changed from 0.11.0 to 1.0.3
  • resolution fixed deleted

[5074] breaks Doctrine_SoftDelete_TestCase: 

php run.php -filter Doctrine_SoftDelete_TestCase

Doctrine_SoftDelete_TestCase : method testTicket1132 failed on line 88 will fail revert to [5071] and everythings works as expected

in my project i don't add any params add all and the problem is that the query is extended by the hook but somehow the added params (array(false) from SoftDelete?) are missing in the query

Changed 9 months ago by jwage

  • status changed from reopened to closed
  • resolution set to fixed

(In [5077]) [1.0] fixes #1132 Fixes regression in r5074

Changed 9 months ago by jwage

That fixes the test case. It was a blatant mistake on my part. I could have sworn that changeset did not introduce any new fails but when I ran the suite again, the test was failing and I found the problem. The 1.1 version was not affected by this changeset.

Changed 9 months ago by tuct

Thx for the fast fix :), also my realworld example is working again

Note: See TracTickets for help on using tickets.