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

Added setCreationOptions() to InvokableFactory #91

Merged

Conversation

kynx
Copy link
Contributor

@kynx kynx commented Jan 27, 2016

I've run into a problem where multiple calls to get('foo', $options) on a plugin manager are not setting the options on subsequent calls. I'm an using an alias to InvokableFactory, as recommended in the v2-v3 migration docs.

This PR addresses that for v2 by adding the MutableCreationOptionsInterface to InvokableFactory.

Note that this only works when the plugin manager has been marked $shareByDefault = false. I am unable to determine if that is expected behaviour.

@kynx
Copy link
Contributor Author

kynx commented Jan 27, 2016

Sigh. This might work for InvokableFactory where we can have different versions in v2 and v3, but not for other packages: MutableCreationOptionsInterface has been removed in v3.

Any suggestions?

@bakura10
Copy link
Contributor

Nooooo not this one ;(. Don't bring back this one.

Any way, you need to call build instead of get to create new instances at each call.

@kynx
Copy link
Contributor Author

kynx commented Jan 27, 2016

@bakura10 Unfortunately SM2 doesn't have a build method. I'm trying to make this forward compatibility thing work.

@kynx kynx changed the title Added MutableCreationOptionsInterface to InvokableFactory Added setCreationOptions() to InvokableFactory Jan 27, 2016
@kynx
Copy link
Contributor Author

kynx commented Jan 27, 2016

Right, for forward compatibility I've changed AbstractServiceManager to duck-type the old MutableCreationOptionsInterface. If found it sets the creationOptions on the factory. The migration process is now:

  • If your factory implements MutableCreationOptionsInterface, remove the interface from the class but leave the setCreationOptions() method. This will ensure the factory works in both v2 and v3.
  • Post migration remove setCreationOptions() and createService() methods

Not pretty, but the best I can think of.

@bakura10
Copy link
Contributor

/me want to create a v4 without compatibility :o.

@kynx
Copy link
Contributor Author

kynx commented Jan 27, 2016

Y'know after struggling with this for the last few days I couldn't agree more :)

@weierophinney
Copy link
Member

Note that this only works when the plugin manager has been marked $shareByDefault = false. I am unable to determine if that is expected behaviour.

It is. If $shareByDefault is enabled, then only the options that are passed for the first retrieval will be honored.

/me want to create a v4 without compatibility

Migration is important. But the nice part is that we only have to think about the current major version to the next, so it should only come up rarely.

@weierophinney weierophinney added this to the 2.7.5 milestone Feb 1, 2016
@weierophinney weierophinney self-assigned this Feb 1, 2016
__CLASS__,
(is_object($creationOptions) ? get_class($creationOptions) : gettype($creationOptions))
));
}
Copy link
Member

Choose a reason for hiding this comment

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

The above are unnecessary, as $creationOptions is type-hinted to array in the signature. Since that's what's defined in the interface, we can just remove the functionality.

I'll take care of it on merge.

@weierophinney
Copy link
Member

@bakura10 Also, this is not bringing back the MutableCreationOptionsInterface; this PR is against the release-2.7 branch. ;-)

@weierophinney weierophinney merged commit 8da8f44 into zendframework:release-2.7 Feb 1, 2016
weierophinney added a commit that referenced this pull request Feb 1, 2016
Added setCreationOptions() to InvokableFactory
weierophinney added a commit that referenced this pull request Feb 1, 2016
weierophinney added a commit that referenced this pull request Feb 1, 2016
@kynx kynx deleted the v2-mutable-creation-options branch February 2, 2016 00:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants