Remove immutability from v3 changes #47
Conversation
|
As for the other issue, I honestly do not have much opinion on that, I won't be against the change.
How is that a problem actually? That sounds sane. Plugin managers come with their own list of pre-configured services, isn't it? |
|
@bakura10 Here's an example: each of zend-navigation, zend-i18n, and zend-form provide additional view helpers that are not in the zend-view HelperPluginManager by default. As such, zend-mvc now fetches a Config class from each, and then uses that to configure the plugin manager. This means calling Previously, this was done in delegator factories, which configured the instance. This can't be done now. There's a few other use cases (we configure the Hal plugin in Apigility within an event). Essentially, plugin managers really need to be mutable, to allow ad-hoc configuration. |
|
I see the issue... as I've already said it's a new example of why we would need a way to merge a config from any library (not only module), and that would solve cleanly this issue. But in the meantime, I'm okay for going back to a mutable manager. |
|
Patch is ready for review! Pinging @ezimuel ! |
|
|
||
| - `configure()`, which accepts the same configuration array as the constructor. | ||
| - `setAlias($alias, $target)` | ||
| - `setInvokableClass($name, $class = null)`; if no `$class` is passed, the |
There was a problem hiding this comment.
Do we really need all those methods ? Just having configure is not enough?
There was a problem hiding this comment.
The methods all proxy to configure(), and reduce BC breaks. I think that's a win.
|
One note: I re-ran the Athletic benchmarks, on both the current develop branch and the one on which this PR is based; there's zero difference! |
| { | ||
| if ($this->allowOverride === false) { |
There was a problem hiding this comment.
oh no... please don't bring back that :(.
There was a problem hiding this comment.
I'm going back and forth on this one. It's available in v2. What's interesting is that it's disabled by default (i.e., you cannot override). In looking through the code, when allow override is disabled, we essentially do checks on service registration to see if the service exists and/or can be created, and then raise an exception. The check I'm doing is a naive check on configure(), and essentially makes it completely immutable at that point.
I guess the question at this point is: do we want the ability to lock the instance down, or not? If not, I'll remove it.
This patch reverts the changes that implemented immutability, and re-instates the various set/add methods used in v2 for configuring the container. Specifically: - `withConfig()` was removed. - `configure()` was made public. - Re-added: - `setAlias()` - `setInvokableClass()` - `setFactory()` - `addAbstractFactory()` - `addDelegator()` - `addInitializer()` - `setService()` - `setShared()` - all of which proxy to `configure()` - Added `mapLazyService($name, $class = null)`, for creating a lazy service mapping from a service name to a given class. - Re-added `setAllowOverride()` for toggling immutability. When toggled on, all calls to `configure()` raise `ContainerModificationsNotAllowedException`. Its counterpart getter, `getAllowOverride()`, was also re-isntated. Tests were added for all new behaviors, and the tests for `withConfig()` were removed.
weierophinney
force-pushed
the
feature/mutability
branch
from
1396c3e
to
4f3680b
Compare
This patch alters how the allow_override flag works in order to better mimic how it was done in v2. This means: - the flag is `false` by default. - the flag only kicks in when new configuration would change a service that has already been fetched and cached internally (i.e., it exists in the `$services` array). The implication is that the following will never be problematic: - abstract factories (as they are a factory of "last resort") - initializers (they only apply to newly created instances) Tests were updated to reflect the changes.
|
I've modified the behavior of the "allow overrides" flag to more closely mimic that of v2 at this point; i.e., if an alias, invokable, factory, delegator, or sharing rule is added, allow overrides is false, and an instance of the named service is already present (the provided service name is mapped in the In discussion with @ezimuel, he suggested potentially more fine-grained settings, with a flag per service indicating ability to override. I think this could be done at a later date without BC ramifications (as the default would be still to have that disallowed by default). While I know @bakura10 is not a fan of the flag, it had specific use cases in v2, particularly to prevent ambiguity about what happens when a change in the state is made. The patch presented in b253d70 keeps the performance improvements introduced for v3, while retaining those protections, and keeping the v2 bootstrap workflow usable. Unfortunately, coveralls won't show me the changes in coverage, and I'm unsure how to improve it at this time (as I have introduced tests for each of the new execution paths created). I think the change is acceptable at this time. |
|
Okay... You are allowed to change it then =).
|
|
@bakura10 ✊ — glad that will work! It's a quite inexpensive check, in the end! |
In refactoring zend-modulemanager, zend-mvc, zend-view, and components with view helper configuration, I've been having to do some rather ugly hacks:
These are due to the fact that the v3 refactor uses immutability as a core feature of the ServiceManager.
I'm starting to question the decision:
I've been doing a little testing, and I've found that the current
configure()logic is actually quite lightweight from a resource perspective: it is not costly in terms of memory, CPU cycles, or time, and, because it takes care of things like alias resolution and abstract factory instantiation, removes the state-based issues we had in v2.This patch does the following:
withConfig().configure()public.configure():setAlias()setFactory()addAbstractFactory()addDelegator()addInitializer()setService()setShared()setAllowOverride()andgetAllowOverride()methods. When disabled,configure()(and hence all methods that proxy to it) will raise aContainerModificationsNotAllowedException.mapLazyService($name, $class = null)method, proxying toconfigure()to allow mapping lazy services after instantiation.Tests were added to cover the new behavior, and documentation updated (including migration documentation).