MC-40313: Eliminate plugins for ActionInterface in Framework #31970
Conversation
|
Hi @engcom-Golf. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
You can find more information about the builds here ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review. For more details, please, review the Magento Contributor Guide documentation. 🕙 You can find the schedule on the Magento Community Calendar page. 📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket. 🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel ✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel |
|
@magento run all tests |
|
@magento run WebAPI Tests, Integration Tests |
|
@magento run all tests |
| * - Correct way for handling requests with `ActionInterface::execute` | ||
| * | ||
| * @param ActionInterface $actionInstance | ||
| * @param HttpRequest $request |
There was a problem hiding this comment.
Why don't you use RequestInterface instead of HttpRequest
Actually variable that you are passing to this method has type RequestInterface
| * Dispatch the controller_action_predispatch events. | ||
| * | ||
| * @param ActionInterface $actionInstance | ||
| * @param HttpRequest $request |
There was a problem hiding this comment.
Why don't you use RequestInterface instead of HttpRequest
Actually variable that you are passing to this method has type RequestInterface
| * @param ActionInterface $actionInstance | ||
| * @param HttpRequest $request | ||
| */ | ||
| private function dispatchPreDispatchEvents(ActionInterface $actionInstance, HttpRequest $request) |
There was a problem hiding this comment.
add return type hinting and annotation
| * Dispatch the controller_action_postdispatch events. | ||
| * | ||
| * @param ActionInterface $actionInstance | ||
| * @param HttpRequest $request |
There was a problem hiding this comment.
Why don't you use RequestInterface instead of HttpRequest
Actually variable that you are passing to this method has type RequestInterface
| * @param ActionInterface $actionInstance | ||
| * @param HttpRequest $request | ||
| */ | ||
| private function dispatchPostDispatchEvents(ActionInterface $actionInstance, HttpRequest $request) |
There was a problem hiding this comment.
add return type hinting and annotation
| */ | ||
| private function isSetActionNoPostDispatchFlag(): bool | ||
| { | ||
| return $this->actionFlag->get('', ActionInterface::FLAG_NO_DISPATCH) || |
There was a problem hiding this comment.
|| should be on the next line
| $request, | ||
| $actionInstance | ||
| ); | ||
| $this->requestValidator->validate($request, $actionInstance); |
There was a problem hiding this comment.
$request parameter should be of type RequestInterface sonce it passed from dispatch() method. Please change parameter type, hinting and annotation
There was a problem hiding this comment.
I have question here ! Is this design fault ?
Previously in code try load design and translate part in catch
In my opinion we should load this part before try catch! Also maybe no need designloader load plugin defined in here to reduce work-load consumption called everytime
https://github.com/magento/magento2/blob/2.4-develop/app/code/Magento/Theme/etc/di.xml#L109
What you guys think?
CC: @kandy @paliarush @engcom-Golf @lbajsarowicz
There was a problem hiding this comment.
$request parameter should be of type RequestInterface sonce it passed from dispatch() method. Please change parameter type, hinting and annotation
Since the method getFullActionName is called on the request, the dependency is actually on the HttpRequest, not the RequestInterface.
Depending on RequestInterface means an interface contract violation.
The options are to either add the method to the RequestInterface (backward incompatible change), or to depend on the HttpRequest (which is simply making the implicit explicit).
|
@magento run all tests |
|
Should we need update description clarify we make this change |
|
@magento run Functional Tests EE |
|
@sidolov @gabrieldagama |
|
The problem here is the public method from The code as it is today is already violating the interface and we should fix it, as discussed with @zakdma, it may be better to re-evaluate the Since would be doing such work, it probably worth looking at other implementations from |
|
I have see some implements with same function name Example 1 here usage of http request Example 2 usage of Magento\Framework\App\RequestInterface |
|
https://github.com/magento/magento2/pull/31970/files#diff-5836c762ee3e1c1bd07900ef34103fc52f0667537b32443371140aabada3ee11R196-198 |
|
@zakdma, an error occurred during the Pull Request import. |
1 similar comment
|
@zakdma, an error occurred during the Pull Request import. |
|
@zakdma, an error occurred during the Pull Request import. |
1 similar comment
|
@zakdma, an error occurred during the Pull Request import. |
|
@zakdma the branch with code successfully imported into |
|
Hi @engcom-Golf, thank you for your contribution! |
Description (*)
Plugins should be used to customize behavior of one module from another module. In this case, plugins that belong to Framework customize class behavior of the same Framework.
This PR moves logic of framework plugins directly to the place where it's expected to execute
Related Pull Requests
Fixed Issues (if relevant)
Manual testing scenarios (*)
smoke test
Questions or comments
Contribution checklist (*)