Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add reset functionality on the Select class #95

Merged
merged 5 commits into from
Aug 18, 2016
Merged

Add reset functionality on the Select class #95

merged 5 commits into from
Aug 18, 2016

Conversation

pmjones
Copy link
Member

@pmjones pmjones commented May 19, 2016

@yespire @golgote @gauthier @harikt --

This PR is based on #84 from @yespire. It combines the resetFrom(), resetFromKey(), and resetTableRefs() into a single resetTables() method, and adds resetBindValues() and resetUnions(). It removes resetLimit(), resetOffset(), resetPage(), because those values are accessible through existing limit(), offset(), and page() methods. Finally, the resetFlags() method is made public instead of protected, and now returns $this.

In summary, the newly-public reset methods are:

  • resetBindValues()
  • resetFlags()
  • resetCols()
  • resetTables()
  • resetWhere()
  • resetGroupBy()
  • resetHaving
  • resetOrderBy()
  • resetUnions()

Thoughts?

@harikt
Copy link
Member

harikt commented May 20, 2016

I am good with this.

Thank you.

@gauthier
Copy link

@pmjones, thanks for having moved so fast on this topic.

I would of course happy to see this PR merged in a release, that would let me remove these awful Reflection* instances I had to use to achieve the very same thing :)

On the other hand, in the PR I made, I took care of allowing to also reset parts from Delete and Update queries. I'm not 100% sure this would as useful as for Select, anyway, from a API consistency point of view, I think that would have been a good thing: if you can reset "where" on Select, why not on "update"?

@yespire
Copy link
Contributor

yespire commented May 20, 2016

@gauthier

I think you have a valid point.

I see there are 2 options:

  1. reduce the reset functions to absolutely necessary ones only, not every reset function has a known use case
  2. keep api consistent, add more reset / clear functions to all query types

for 1. there probably need be some examples for users of package to understand the use cases of the allowed reset functions.
for 2. this is less opinionated, more flexible

One counter argument for 2: most users, most project may never need to reset part of update+delete query builder object -- the reason is that these 2 only needs to execute once, business logics are normally consolidated first before being used for query builder. Therefore reset is unlikely to happen for them.

@pmjones
Copy link
Member Author

pmjones commented May 20, 2016

@gauthier I too find it easy to imagine that users may want to reset parts of an INSERT or UPDATE; however, I have a hard time imagining exactly why they will want to do the reset. (I have not, myself, ever had the need.)

@yespire Of the new reset() functions on SELECT, only about half of them have been asked-for (especially resetCols(), so we can re-use a query to get a row-count). The resetTables() method, for example, has not been requested anywhere. Perhaps removing, or even just making protected, the methods that have not been asked-for elsewhere is the right thing.

Further thoughts?

@pmjones
Copy link
Member Author

pmjones commented May 20, 2016

(FWIW it seems like only resetCols() would really be needed in the latter case.)

@pavarnos
Copy link
Contributor

Looking good. Is it possible to make a release soon? ... would be really handy for mysql 5.7 with sql_mode=only_full_group_by and generic paginator code that can take an Aura Select

@compwright
Copy link

MySQL chokes if you have an ORDER BY when doing a COUNT(*) query. So there is a possible need for a resetOrderBy() method when paginating.

@harikt
Copy link
Member

harikt commented Aug 2, 2016

@compwright resetOrderBy is already here or did I missed something.

@compwright
Copy link

@harikt I was responding to @pmjones observation that some of these may not be needed. I would err on the side of flexibility because you can't really predict the ways queries may need to morph at runtime.

@harikt
Copy link
Member

harikt commented Aug 3, 2016

@compwright cool. I missed :) . Thanks for your thoughts.

@harikt
Copy link
Member

harikt commented Aug 17, 2016

@pmjones could you merge this and make a release?

@harikt
Copy link
Member

harikt commented Aug 19, 2016

@pmjones any reason you have not removed the branch ?

@harikt
Copy link
Member

harikt commented Sep 3, 2016

Deleting the branch.

@harikt harikt deleted the resets branch September 3, 2016 04:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants