Skip to content

Added native query adapter - #167

Open
Nek- wants to merge 3 commits into
whiteoctober:masterfrom
Nek-:feature/native-query-adapter
Open

Added native query adapter#167
Nek- wants to merge 3 commits into
whiteoctober:masterfrom
Nek-:feature/native-query-adapter

Conversation

@Nek-

@Nek- Nek- commented Jan 2, 2015

Copy link
Copy Markdown

No description provided.

@Nek-
Nek- force-pushed the feature/native-query-adapter branch from 676d9b3 to 3b2e6de Compare January 2, 2015 13:20
Doc update

Fixed doc mistake
@Nek-
Nek- force-pushed the feature/native-query-adapter branch 3 times, most recently from 960082b to 0c5c834 Compare January 2, 2015 13:40
@pablodip

pablodip commented Jan 2, 2015

Copy link
Copy Markdown
Contributor

Could anyone related with DoctrineORM review this? CC/ @stof :)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will fail when you have a sub-query in the SELECT part.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Nek- Nek- Jan 2, 2015

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Nek-, you've explained why you'd like the countQueryBuilder method to always be called (even if a modifier is provided), so thank you for that, but I'm not sure you've addressed @stof's concerns that the implentnation of that method will break in a lot of cases.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.. :')

@Nek-

Nek- commented Jan 26, 2015

Copy link
Copy Markdown
Author

Updated, ping @stof

Fixed things after review

Fixed tests
@Nek-
Nek- force-pushed the feature/native-query-adapter branch from 0921030 to a826fc6 Compare January 26, 2015 11:20
@Gemorroj

Copy link
Copy Markdown

any updates?

@astronati

Copy link
Copy Markdown

@Nek- @stof Any update?
I'm having the same need in order to use ResultSetMappingBuilder

@stof

stof commented Jan 16, 2019

Copy link
Copy Markdown
Contributor

I'm not maintainer of this library

@Nek-

Nek- commented Jan 16, 2019

Copy link
Copy Markdown
Author

Not either. Maintainer edit enabled.

@sampart

sampart commented Jan 21, 2019

Copy link
Copy Markdown
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants