This repository has been archived by the owner on Jan 8, 2020. It is now read-only.
Remove SM-Aware requirement from Forward plugin #4330
Merged
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
- This was the old way of getting a controller to forward to:
- Grab the currently set controller
- Grab the service locator from the controller
- Grab the ControllerLoader service from the service locator
- Grab the new controller from the ControllerLoader service
- New way:
- ForwardFactory injects the ControllerManager instance into the
Forward instance on creation.
- Forward plugin pulls the new controller from the composed
ControllerManager.
Better separation of concerns, better usage of Dependency Injection,
controllers no longer need to be ServiceManagerAware in order to forward
(they just need the PluginManager injected).
|
👍 |
|
great! |
| * @param ControllerManager $locator | ||
| * @return self | ||
| */ | ||
| public function setLocator(ControllerManager $locator) |
There was a problem hiding this comment.
Maybe setControllerPluginManager is a better name ? It reduces confusion that "locator" could be THE locator (although, I agree, it's still a locator)
There was a problem hiding this comment.
I did this to be symmetric with the already existing getLocator() method. I'd rather be consistent here.
There was a problem hiding this comment.
@weierophinney is this setter actually needed to be public?
There was a problem hiding this comment.
@Ocramius No, it's not -- that's a remnant from before I was injecting it via the constructor. I'll change that shortly.
- Use constructor injection for getting ControllerManager into Forward plugin (per @bakura10) - Remove test case that's no longer applicable (we can now forward to any controller!) - Fix test cases that were instantiating the Forward plugin directly to inject the ControllerManager
|
Superb, I think this will make the controller much clearer :) :+1 |
| * | ||
| * @return ControllerManager | ||
| */ | ||
| public function getLocator() |
- Since we're doing constructor injection, the setter is not necessary, and neither is the getter. As the setter did not exist previously, and the getter was protected previously, they can safely be removed. - Renamed $locator to $controllers to make the name more semantic.
akrabat
added a commit
that referenced
this pull request
Apr 30, 2013
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.
This is based on discussions in #4314 - essentially, the main reason that controllers are ServiceManagerAware is because it was the only way to have workflow-aware plugins such as the Forward plugin. Or so we thought.
On analysis, it's far simpler and easier to inject the ControllerManager instance directly into the Forward plugin on creation, as it allows us to remove quite some code, and remove the requirement that controllers be ServiceManagerAware in order to forward.
This PR modifies the Forward plugin to accept a ControllerManager instance via a setter, and adds a ForwardFactory that does this. Tests have been modified as the internal workflow was able to change and be simplified (much of the work done was validating the controller, and this is handled by the ControllerManager now).