SQLFilters enahancements #1054
SQLFilters enahancements #1054
Conversation
Make connection available in filters Add test for the changes
|
Hello, thank you for creating this pull request. I have automatically opened an issue http://www.doctrine-project.org/jira/browse/DDC-3161 We use Jira to track the state of pull requests and the versions they got |
| */ | ||
| final public function __construct(EntityManager $em) | ||
| final public function __construct(EntityManagerInterface $em) |
There was a problem hiding this comment.
changing the typehint is a BC break
|
Why? This is a final method? |
|
Reverted |
|
hmm, indeed, for a final method, it is not a BC break as the typehint gets more generic. But this should be done separately anyway. |
|
@v3labs why not using the filter parameters to quote them ? they get quoted by the SQLFilter class |
|
It's not only quoting. It's more about sql expressions that get generated by methods like: You need the database platform class which cannot be accessed without access to the connection. I guess only a subset of these methods can be proxied in the SQLFilter class but it seems kind of pointless. |
|
Any feedback? |
|
Seems sane to me. Ping @asm89 |
| $reflMethod = new \ReflectionMethod('Doctrine\ORM\Query\Filter\SQLFilter', 'getConnection'); | ||
| $reflMethod->setAccessible(true); | ||
|
|
||
| $this->assertTrue($reflMethod->invoke($filter) === $conn); |
There was a problem hiding this comment.
I think this should rather be $this->assertSame($conn, $reflMethod->invoke($filter));
|
Besides the assertion optimization this looks reasonable to me. Ping @asm89 |
|
I just fixed the assertion issue. |
SQL filters in Doctrine are a bit too limited for my taste.
This pull request makes the current connection available inside sql filter classes.
The connection is needed to quote identifiers or values and generate sql statements for the current database. (sample use-case https://github.com/Atlantic18/DoctrineExtensions/blob/master/lib/Gedmo/SoftDeleteable/Filter/SoftDeleteableFilter.php)
Currently you can access the connection only through reflection which let's face it - is a hack.
I've also made the class depend on EntityManagerInterface instead of EntityManager.
There's a test for the new method.