Added native query adapter #167
Conversation
Nek-
force-pushed
the
feature/native-query-adapter
branch
from
676d9b3 to
3b2e6de
Compare
Doc update Fixed doc mistake
Nek-
force-pushed
the
feature/native-query-adapter
branch
3 times, most recently
from
960082b to
0c5c834
Compare
|
Could anyone related with DoctrineORM review this? CC/ @stof :) |
|
|
||
| private function countQueryBuilder(NativeQuery $query) | ||
| { | ||
| $sql = explode(' FROM ', $query->getSql()); |
There was a problem hiding this comment.
This will fail when you have a sub-query in the SELECT part.
There was a problem hiding this comment.
This will fail in a lot of case actually. And the implementation forces to apply this even when providing a custom modifier, meaning there is no way to get rid of this broken implementation.
There was a problem hiding this comment.
At first the method countQueryBuilder was originally called on the else statement of this if https://github.com/whiteoctober/Pagerfanta/pull/167/files#diff-47292a5a239d0185dbe662f2f1b0ed81R70 .
But then I wrote the test and I noticed that having the result set mapping that corresponds to the count is more than useful. That's why I changed it to make it like that.
There was a problem hiding this comment.
There was a problem hiding this comment.
The best answer I can give here is the detection of too complex queries and throw an exception for these cases. Otherwise good thing to do is to build an SQL analyzer.. :')
|
Updated, ping @stof |
Fixed things after review Fixed tests
Nek-
force-pushed
the
feature/native-query-adapter
branch
from
0921030 to
a826fc6
Compare
|
any updates? |
|
I'm not maintainer of this library |
|
Not either. Maintainer edit enabled. |
|
Thanks for your contributions, @Nek-, and for your review work, @stof. I don't have the Doctrine knowledge of someone like @stof, but there were a couple of comments from the original review which didn't look like they'd be addressed (or only partially addressed). I've added comments in the relevant thread above to show what I think's needed. @stof would you be happy to re-review if @Nek- does address your original concerns? As I say, you have more knowledge in this area than I do! Thanks again both. |
No description provided.