Fix parameter name comparison in AbstractQuery regarding different types (fixes #6699) #6705
Conversation
Majkl578
force-pushed
the
ticket/6699
branch
3 times, most recently
from
ab436ea to
708f8c0
Compare
Majkl578
changed the title
|
Reworded & fix + more tests added. |
|
You have only updated the |
lib/Doctrine/ORM/AbstractQuery.php
Outdated
| @@ -323,8 +323,8 @@ public function getParameter($key) | |||
| $filteredParameters = $this->parameters->filter( | |||
| function ($parameter) use ($key) | |||
| @@ -369,17 +369,10 @@ public function setParameters($parameters) | |||
| */ | |||
| public function setParameter($key, $value, $type = null) | |||
| { | |||
| $filteredParameters = $this->parameters->filter( | |||
There was a problem hiding this comment.
Nice simplification here, do we have any impact on performance (due to the additional method call)?
There was a problem hiding this comment.
Just tested, the answer is: we have no performance impact
| use Doctrine\Tests\OrmFunctionalTestCase; | ||
|
|
||
| /** | ||
| * @group #6699 |
There was a problem hiding this comment.
We're using the ticket id without the # everywhere else, we should keep it consistent here too
| @@ -303,4 +304,65 @@ public function testSelectFromSubquery() | |||
| $this->expectExceptionMessage('Subquery'); | |||
| $query->getSQL(); | |||
| } | |||
|
|
|||
| /** | |||
| * @group #6699 | |||
There was a problem hiding this comment.
We're using the ticket id without the # everywhere else, we should keep it consistent here too
| } | ||
|
|
||
| /** | ||
| * @group #6699 |
There was a problem hiding this comment.
We're using the ticket id without the # everywhere else, we should keep it consistent here too
|
|
||
| public function testMixedParametersWithZeroNumber() | ||
| { | ||
| $qb = $this->_em->createQueryBuilder() |
There was a problem hiding this comment.
We can simplify this bit, since the query builder reference is not really needed, like:
$query = $this->_em->createQueryBuilder()
->select('u')
->from(CmsUser::class, 'u')
->andWhere('u.username = :username')
->andWhere('u.id = ?0')
->getQuery();| parent::setUp(); | ||
| } | ||
|
|
||
| public function testMixedParametersWithZeroNumber() |
| */ | ||
| class GH6699Test extends OrmFunctionalTestCase | ||
| { | ||
| protected function setUp() |
| /** | ||
| * @group #6699 | ||
| */ | ||
| public function testSetParameterWithTypeJugglingWorks() |
| */ | ||
| public function testSetParameterWithTypeJugglingWorks() | ||
| { | ||
| $query = $this->_em->createQuery("select u from Doctrine\Tests\Models\CMS\CmsUser u where u.id != ?0 and u.username = :name"); |
There was a problem hiding this comment.
Could use ' . CmsUser::class . ' to keep consistency
lcobucci
force-pushed
the
ticket/6699
branch
from
708f8c0 to
dc2fccf
Compare
lcobucci
force-pushed
the
ticket/6699
branch
from
dc2fccf to
b8fd708
Compare
|
@Majkl578 🚢 and backported to Thanks! |
|
We should probably copy this behavior to AbstractQuery::setParameter() as well (thanks for spotting @Seb33300). |
I've applied it there as well |
|
@lcobucci Cool, thanks! |
AbstractQuery::setParameter() uses weak comparison to compare parameter names and decide whether to update existing or add new parameter. AbstractQuery::getParameter() does so as well.
This is problematic for parameter named
0since strings are downcasted to0as well, making it incorrectly assume that i.e. the0and"foo"are the same parameter.In most cases, cast is not needed and we would ideally want to prevent it, thus added identity check first and only if it fails, string cast comparison is attempted.
Fixes #6699.