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

Make GatewayFactory::create method static #1

Closed
wants to merge 1 commit into from

Conversation

alfaproject
Copy link

Factory methods should be static. This will clean up a lot of notices on my servers. (:

@amacneil
Copy link
Member

From Omnipay v2 you can use Omnipay::create() if you want static access to the factory methods. The factory class itself is no longer static to facilitate easier testing and support those who want to use dependency injection in their own code. Check out the release notes for more info.

@amacneil amacneil closed this Jan 17, 2014
@alfaproject
Copy link
Author

While I do understand that we can use the new method, that doesn't invalidate the fact that GatewayFactory::create, and actually GatewayFactory::getSupportedGateways should be static. The reason being that they don't access any instance variables.

You are basically forcing the consumer of those methods to create an instance of GatewayFactory (a new object), just to use them. If you really want to deprecate it, then you should mark it as obsolete and let the consumer deal with the hurdle. Or at the very least also add something in the phpdoc block to 'see' the new method that should be used instead.

That's my 2 cents, but it's a good practise in OOP. I'll keep it static in my fork, and actually mark getSupportedGateways static as well, together with some new documentation. Surely, you can't expect everyone to read the README for these kind of things.

@amacneil
Copy link
Member

As I said, there are legitimate reasons for making GatewayFactory an instance (makes dependency injection & testing much easier). If you want a static factory you can easily use the Omnipay class instead.

This was a breaking API change and therefore following semver was introduced in a new major version (2.0). I absolutely do expect people to read the release notes before installing a new major version of a library (or at least have test coverage to ensure that any API changes are caught).

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.

2 participants