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 PheanstalkInterface #67

Merged
merged 5 commits into from Mar 13, 2013
Merged

add PheanstalkInterface #67

merged 5 commits into from Mar 13, 2013

Conversation

armetiz
Copy link
Contributor

@armetiz armetiz commented Mar 8, 2013

No description provided.

@pda
Copy link
Collaborator

pda commented Mar 12, 2013

Thanks for the contribution.

There's some failing tests which need to be fixed:
https://travis-ci.org/pda/pheanstalk/jobs/5346111

Declaration of Pheanstalk_Pheanstalk::setConnection() must be compatible with that of Pheanstalk_PheanstalkInterface::setConnection()

Looks like the implementation needs the Pheanstalk_Connection type hint.

Cheers,
Paul

@armetiz
Copy link
Contributor Author

armetiz commented Mar 12, 2013

Shame on me, I have not launched the tests.
I'll fix this in few hours.

@armetiz
Copy link
Contributor Author

armetiz commented Mar 12, 2013

Fixed !

@pda
Copy link
Collaborator

pda commented Mar 12, 2013

Does anybody have any objections to adding Pheanstalk_PheanstalkInterface, implemented by Pheanstalk_Pheanstalk?

If not, I'll merge this in and include it in v2.0.0-rc2.

Pinging recent pull-requesters: @mnapoli @phansys @jimbojsb

@jimbojsb
Copy link
Collaborator

Seems fine to me. To what end though?

Sent from my iPhone

On Mar 12, 2013, at 6:46 PM, Paul Annesley notifications@github.com wrote:

Does anybody have any objections to adding Pheanstalk_PheanstalkInterface,
implemented by Pheanstalk_Pheanstalk?

If not, I'll merge this in and include it in v2.0.0-rc2.

Pinging recent pull-requesters: @mnapoli https://github.com/mnapoli
@phansys https://github.com/phansys @jimbojsbhttps://github.com/jimbojsb


Reply to this email directly or view it on
GitHubhttps://github.com//pull/67#issuecomment-14813388
.

@pda
Copy link
Collaborator

pda commented Mar 12, 2013

To what end though?

Good question.

My guess is that people prefer to type-hint and mock interfaces rather than implementations…
Maybe @armetiz can give more specific reasons.

@mnapoli
Copy link
Contributor

mnapoli commented Mar 13, 2013

My guess is that people prefer to type-hint and mock interfaces rather than implementations…

Yep that's a good idea, even though I'd rather type-hint on a BeanstalkdService or QueueService implemented by Pheanstalk so that it makes sense if one day I want to use another Beanstalkd library, or another queue system. But these (BeanstalkdService or QueueService) are out of the scope of Pheanstalk I'd say.

That reminds me: is there a PHP 5.3 version of Pheanstalk with namespaces planned? Because then the class names would probably be more like:

  • Pheanstalk\Pheanstalk
  • Pheanstalk\PheanstalkInterface (because Interface is a reserved keyword, and it makes more sense anyway)

So I'm not a fan of long class names (like Pheanstalk_PheanstalkInterface) but if a PHP 5.3 version is around the corner, maybe the name of the interface can be changed so that it minimize the migration work later.

@pda
Copy link
Collaborator

pda commented Mar 13, 2013

That reminds me: is there a PHP 5.3 version of Pheanstalk with namespaces planned?

There's been at least one fork/branch with namespaces, although I imagine it's been left behind with recent changes.

Given the library is compatible with pre-namespace versions of PHP (5.2 as far as I know), I can see more harm than good coming from namespacing it. That said, I also dislike long class names, so maybe we can get v2.0.0 tagged and released, and then convert to namespaces for a future v3.0.0 release.

maybe the name of the interface can be changed so that it minimize the migration work later.

One of us is confused, and it might be me :) But I believe @armetiz has gone with the future-namespace-compatible format of Pheanstalk_PheanstalkInterface, right?

@mnapoli
Copy link
Contributor

mnapoli commented Mar 13, 2013

Ah nevermind it's the morning for me :p indeed the name is already Pheanstalk_PheanstalkInterface I read differently at first.

And good idea to have namespaces in a future *major *release, that makes more sense because it will break everything.

@armetiz
Copy link
Contributor Author

armetiz commented Mar 13, 2013

Hi there.
I want to create a PheanstalkProxy in my library that implements PheanstalkInterface rather than extends Pheantalk.

Work with interfaces is really a good practice. It's allow more flexibility for the end-user.

pda added a commit that referenced this pull request Mar 13, 2013
The implied interface of `Pheanstalk_Pheanstalk` has been extracted into
`Pheanstalk_PheanstalkInterface`, which `Pheanstalk_Pheanstalk` now implements.
@pda pda merged commit 7407c50 into pheanstalk:master Mar 13, 2013
@armetiz armetiz deleted the interface branch March 13, 2013 08:20
@phansys
Copy link
Contributor

phansys commented Mar 13, 2013

It's OK for me.
Thank you!

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.

None yet

5 participants