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

getStored() deprecation in ResultSetBase #146

Open
vchebotarev opened this issue Apr 16, 2018 · 5 comments
Open

getStored() deprecation in ResultSetBase #146

vchebotarev opened this issue Apr 16, 2018 · 5 comments

Comments

@vchebotarev
Copy link
Contributor

And I have a question about getStored deprecation
It was quiet hard to understand its code, so then i recognized that there is no need to remove it
I have a suggection

  • reorganize result interfaces to have single responsobilities
    -- ResultSetInterface with store() and getStored()
    -- MultiResultSetInterface extends ResultSetInterface
    -- SingleResultSetInterface extends ResultSetInterface
  • rename getStored() to getResults()
  • rename store() to collectResults()
    and them it won't be so complicated to understand waht is going on

what do you think ?

@oohnoitz
Copy link
Contributor

reorganize result interfaces to have single responsobilities

I don't have any strong preference, but if you can ship this as a separate PR that I can take a look at we can see where it goes from there.

rename getStored() to getResults()

I believe the whole "stored" keyword was being passed around because mysqli uses that same keyword for interacting with the result set returned with that driver. We also have a getResult method in src/SphinxQL so we would need to take in consideration some of the chaining syntax. Take a look at the following snippet in the test, which is valid chained syntax which returns the same result. I can't remember how it works with MultiResultSet off the top of my head though since I haven't touched/used this library in a long time.

$describe = $describe
->getResult()
->getStored();

rename store() to collectResults()

I do think this name reflects what it's actually doing. It's just that we've been going off of the way mysqli named their methods and simply kept with it, but it shouldn't be the deciding factor since we technically should work as a simple wrapper that works with multiple drivers. In other words, I'd be fine with this change.

@vchebotarev
Copy link
Contributor Author

Oh, i understood. This issue is a bad suggestion.
Everything got mixed in my head - because in result sets mixed to much logic and namings don't make clear clear what is really going on.
I think a little bit about it in general, don't close issue for some time ...

@vchebotarev
Copy link
Contributor Author

Well
I thought a lot about it ...
What do you think about doctrine/dbal and in mysqli and pdo drivers?
It is the most popular package for databases using
I am thinking about some lib which will adoptations of doctrine/dbal for sphinx using inheritance or decoration to close unsupperted features and adding new to cover all sphinxQL features
It will relieve us from result sets support as a headache during inveting bicycle ...
This package wiil be only query builder not connection/fetching/execution manager
divide et impera not support )

@oohnoitz
Copy link
Contributor

oohnoitz commented May 26, 2018

I was a bit busy the past few weeks and forgot about this issue. I do think this might be worth looking into so that we don't have to manage connections which has been a bit of a hassle and only focus on our goal of providing a "Query Builder" for the SphinxQL syntax. This may also end up resolving #118 which has been open for quite some time.

@woxxy
Copy link
Member

woxxy commented May 27, 2018

As long as you can create a new Connection class that used Doctrine DBAL, you should be good to go. Sadly we can't completely separate this library from the Connection as we need it to escape strings.

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

No branches or pull requests

3 participants