This repository has been archived by the owner on Feb 6, 2020. It is now read-only.
[BUG] Own factories for plugins wont reset creationOptions #205
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
instead of `InvokableFactory` with `creationOptions`
|
I don't understand what you mean by "an own factory". Do you mean that "creation options will not be reset between invocations of a custom factory"? |
|
Exactly. Own factories are not being reset since they dont have special handling like |
[BUG] Own factories for plugins wont reset creationOptions
The issue revealed in #205 unveiled a few issues: - creation options were not being reset in factories when the same service was requested again without options. - services created with options were being cached, when they should not be. I added several tests to cover the different aberrant behaviors, and then fixed the issue. The fix required a few changes: - If non-empty options are passed to `get()`, we check to see if a shared instance already exists. If so, we cache the shared instance and remove its instance from the container state in order to force creation of a new instance. Once done, we reset the container state to store the original shared instance. - If non-empty options are passed, the instance generated will now never be shared. - When `createServiceViaCallback()` is called, we do not check for non-empty creation options if the `setCreationOptions()` method exists; we always pass them. Further, we always cast non-array creation options to an empty array.
- zendframework/zend-code 3.0 is PHP 7.1+ only, and makes tests on this release branch fail due to syntax issues. - doctrine/instantiator 1.1.0 is PHP 7.1+ only, and makes tests on this release branch fail due to syntax issues.
|
@boesing I was able to take your tests as a starting point, and complete the bugfix. It went a bit deeper than you might have known - we were storing instances that had creation options, when we shouldn't have been! I'll be cutting the new 2.7 release shortly. |
weierophinney
added a commit
that referenced
this pull request
Nov 27, 2017
weierophinney
added a commit
that referenced
this pull request
Nov 27, 2017
|
Appears like this PR is causing the regression of this old issue again :/ |
weierophinney
added a commit
to weierophinney/zend-servicemanager
that referenced
this pull request
Dec 5, 2017
…pty array Per zendframework#159 (comment), the updates in zendframework#205 broke a previously fixed edge case whereby `AbstractPluginManager::createServiceViaCallback()` would inject a `null` value to a factory's `setCreationOptions()` method if no instance options were provided; it instead passed an empty array, which broke a number of plugins that relied on a null value being present. This patch does the following: - Adds a regression test for the behavior. - Modifies `AbstractPluginManager::createServiceViaCallback()` to have three distinct behaviors surrounding creation options, in the following order: - if the factory is an `InvokableFactory`, it tests if the creation options are an array or non-empty; if not, it passes a `null` value to `setCreationOptions()`. - if the factory is an `MutableCreationOptionsInterface`, it tests if the creation options are an array or non-empty; if not, it passes an empty array to `setCreationOptions()`. - if the factory implements the `setCreationOptions()` and the creation options are a non-array or non-empty, it reflects the method to determine if the options argument has a default value, using that if present, or an empty array if not.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If you use an own factory, the creationOptions wont get resetted.
There is a check, that if the factory is an
InvokableFactory, that$factory->setCreationOptions(null)is being called, because it has thearray $optionsas optional parameter.The problem is, that the method itself is not documented and therefore some factories might have required
$optionsparameter in the method declaration.Any ideas of how we could reset those options for factories with required
$optionsparameter?Or should those factories handle the reset of the
$creationOptionsproperty itself?