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

Remove ServiceLocatorAwareInterface from table gateways. #910

Merged
merged 1 commit into from Feb 10, 2017

Conversation

Projects
None yet
2 participants
@demiankatz
Copy link
Member

commented Feb 8, 2017

TODO

  • Run full test suite with MySQL.
  • Run full test suite with PostgreSQL.

@demiankatz demiankatz force-pushed the db-no-servicelocator branch 2 times, most recently from 7c31511 to c3c5482 Feb 8, 2017

@demiankatz demiankatz requested review from crhallberg and EreMaijala Feb 8, 2017

@demiankatz

This comment has been minimized.

Copy link
Member Author

commented Feb 8, 2017

Here's some more ServiceLocatorAwareInterface refactoring to complement #909. Once again, a few more eyes on the problem would be appreciated to make sure I haven't done anything foolish. From @EreMaijala, I'm particularly interested in whether this will break any hard-to-test features like the ExternalSessions table. So far, all commonly-tested features seem to be working as normal.

@demiankatz demiankatz force-pushed the db-no-servicelocator branch from c3c5482 to e04d634 Feb 8, 2017

@EreMaijala
Copy link
Contributor

left a comment

If I have in Finna\Db\Table namespace a Factory class that inherits this one, will NAMESPACE be Finna\Db\Table? I couldn't quite find that out in the documentation.

@demiankatz

This comment has been minimized.

Copy link
Member Author

commented Feb 10, 2017

No, I think that NAMESPACE is constant to the file, even if you subclass it. If you need to build objects using a different namespace, we should either add a parameter to the function to allow a namespace override, or else you should duplicate the method in your own factory.

I also don't claim that the __callStatic() approach I'm using in the factory here is really the best way to do this -- it was just a simple approach to get everything working quickly. The most common practice these days seems to be one factory class per class. I still think the idea of having a single collection of static factory methods has some benefits, but I could certainly be persuaded to bring VuFInd in line with other projects in the future, especially if changes to the ZF 3.0 ServiceManager require a rewrite of all of the factories (which may be a possibility).

@EreMaijala

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2017

Well, for now I think it would suffice for us to make __callStatic only prepend the namespace if there are no backslashes in $name. What do you think?

@demiankatz

This comment has been minimized.

Copy link
Member Author

commented Feb 10, 2017

That sounds like a reasonable revision. I'll work on that shortly, unless you already have an implementation you can push up.

@EreMaijala

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2017

I haven't done anything yet, so please go ahead. Thanks!

@demiankatz demiankatz force-pushed the db-no-servicelocator branch from e04d634 to 276434d Feb 10, 2017

@demiankatz

This comment has been minimized.

Copy link
Member Author

commented Feb 10, 2017

@EreMaijala, I've just revised the commit to include a better version of the factory. Now, instead of using __callStatic everywhere, I've created a getGenericTable method that supports either a relative or absolute class name. __callStatic is now a lightweight wrapper around getGenericTable (as are all of the other more explicit factories). In addition to supporting your use case, this also better isolates the messy "magic" method name munging from the cleaner process of actually building the table objects. Thanks again for the input; please let me know if you have trouble with this, or any other concerns.

@EreMaijala
Copy link
Contributor

left a comment

Looks good to me.

@demiankatz demiankatz merged commit aaa36d7 into master Feb 10, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@demiankatz demiankatz deleted the db-no-servicelocator branch Sep 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.