Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

Removed the zend-servicemanager usage #47

Merged

Conversation

ezimuel
Copy link
Contributor

@ezimuel ezimuel commented Dec 7, 2015

This PR removes the usage of zend-servicemanager using a custom SmtpPluginManager. This removal simplify the usage of the component.

@weierophinney weierophinney added this to the 3.0.0 milestone Dec 7, 2015
@weierophinney weierophinney self-assigned this Dec 7, 2015
@@ -14,15 +14,14 @@
},
"require": {
"php": ">=5.5",
"zendframework/zend-crypt": "~2.5",
"zendframework/zend-crypt": "dev-develop as 2.6.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're removing zend-servicemanager as a dep, the change is essentially backwards compatible.

Couldn't this dependency be ^2.5 || ^3.0? That would make it possible to keep this component in the 2.X series.

@weierophinney weierophinney modified the milestones: 3.0.0, 2.6.0 Dec 7, 2015
@weierophinney
Copy link
Member

With the suggested changes, we can target 2.6.0 instead. 😄

@gianarb
Copy link

gianarb commented Dec 14, 2015

👍 👍

@ezimuel
Copy link
Contributor Author

ezimuel commented Jan 4, 2016

@weierophinney the PR is ready to be merged.

@ezimuel ezimuel closed this Jan 4, 2016
@ezimuel ezimuel reopened this Jan 4, 2016
@@ -14,15 +14,15 @@
},
"require": {
"php": ">=5.5",
"zendframework/zend-crypt": "~2.5",
"zendframework/zend-crypt": "^2.5 || ^3.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

|| ? Should be a simple |

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Maks3w the Composer documentation for that is inconsistent, and uses both. As the parser accepts either, I think using || makes sense, for its affiliation with the language itself.

@weierophinney weierophinney merged commit 8c638f5 into zendframework:develop Jan 7, 2016
weierophinney added a commit that referenced this pull request Jan 7, 2016
weierophinney added a commit that referenced this pull request Jan 7, 2016
weierophinney added a commit that referenced this pull request Jan 7, 2016
@Slamdunk
Copy link
Contributor

@weierophinney AFAIK Zend Framework is following SemVer, isn't it?

Because I just updated my dependancies, and everything broke up because API totally changed in a non BC way.

This PR should be tagged 3.0.0, not 2.6.0, like what it was done to other 2.6 components.

@weierophinney
Copy link
Member

@Slamdunk What breakage are you seeing in this component, specifically?

@Slamdunk
Copy link
Contributor

The fact that all the public method of AbstractPluginManager and ServiceManager are now missing.

Or in the SemVer the API are only the ones listed in an Interface, in this case ServiceLocatorInterface?

@weierophinney
Copy link
Member

@Slamdunk You're missing the point of my question: has any code of yours specifically broken? If so, what specifically? Any code snippets of something you were actually using that no longer works would be useful.

The plugin manager implementation in this component was by-and-large an implementation detail, which is why we felt it was safe to refactor it to be standalone; however, I'd like to see if people were relying on the fact that it was an AbstractPluginManager implementation specifically, and, if so, how they were using it.

@Slamdunk
Copy link
Contributor

Let me preface this comment by saying that refactoring my code to this PR was a matter of seconds; my speech was more a matter of theory on SemVer than a complain.

As posted on #27 I need a custom Transport\Smtp and Protocol\Smtp to face the timeout issues on long running scripts.
So I used to pull the Protocol\SmtpPluginManager and call the setInvokableClass to use my custom implementation of Protocol\Smtp.

After the update this behaviour produced a Fatal error, and now I'm forced to subclass the Protocol\SmtpPluginManager.

SemVer says:

  1. MAJOR version when you make incompatible API changes
  2. MINOR version when you add functionality in a backwards-compatible manner

Well, a Fatal error from 2.5.* to 2.6.* it not seems a backwards-compatible change 😁

@weierophinney
Copy link
Member

@Slamdunk You've provided me the information I was looking for: you were using the features inherited from AbstractPluginManager, which means that while we were treating it as an internal implementation detail, users were consuming it as part of the public API. As such, yes, this is a BC break, and we need to revisit it.

Thanks for providing me the usage detail!

weierophinney added a commit to weierophinney/zend-mail that referenced this pull request Feb 24, 2016
This patch re-implements the SmtpPluginManager as a zend-servicemanager
AbstractPluginManager, as was the behavior before 2.6.0. As reported on zendframework#47,
making the implementation standalone was a backwards compatibility
break, as it was not an internal implementation only, but also the
documented mechanism for providing additional featuresets and/or
overriding the functionality of existing features.

To accomplish this, the following changes were made:

- Added a development requirement on zend-servicemanager `^2.7.5 || ^3.0.3`.
- Added a `suggest` section, recommending zend-servicemanager when using
  the SMTP transport.
- Added compatibility tests for the `SmtpPluginManager`, to assure equal
  behavior between v2 and v3 of zend-servicemanager.
- Updated `SmtpPluginManager` to extend `AbstractPluginManager` and
  define aliases and factories for the shipped protocol plugins.
@weierophinney
Copy link
Member

@Slamdunk If you update to zend-mail 2.6.1, we've restored the AbstractPluginManager implementation.

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

Successfully merging this pull request may close these issues.

5 participants