Adding a validation before adding or executing layout generator class. #22324

tiagosampaio · 2019-04-13T15:46:47Z

Before adding or executing any layout generator it's important to validate the class before.

Fixed Issues (if relevant)

It fixes no issues.

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

m2-assistant · 2019-04-13T15:46:49Z

Hi @tiagosampaio. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento-engcom-team give me test instance - deploy test instance based on PR changes
@magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

lib/internal/Magento/Framework/View/Layout/GeneratorPool.php

magento-engcom-team · 2019-04-14T12:54:36Z

Hi @dmytro-ch, thank you for the review.
ENGCOM-4750 has been created to process this Pull Request

orlangur · 2019-04-14T22:55:26Z

lib/internal/Magento/Framework/View/Layout/GeneratorPool.php

@@ -77,6 +77,10 @@ public function process(Reader\Context $readerContext, Generator\Context $genera
    {
        $this->buildStructure($readerContext->getScheduledStructure(), $generatorContext->getStructure());
        foreach ($this->generators as $generator) {
+            if (!$this->validateGenerator($generator)) {
+                continue;


Fixed Issues (if relevant)
It fixes no issues.

What kind of issue it fixes then? What happened before if corresponding interface is not implemented?

Hi @orlangur.

If any 3rd party module overrides or include a generator in GeneratorPool and doesn't declare the methods getType() and process() the user will be given an exception. That's why I think it's a good idea to force the implementation of GeneratorInterface.

Examples:

Call to undefined method Any\Class::getType()
Call to undefined method Magento\Framework\View\Page\Config\Generator\Head::process()

@tiagosampaio so user already receives an exception and by this change you actually hide it. What should really be done is throwing exception in case of incorrect interface and only in addGenerators method.

@orlangur you're right. I'll do it.

lib/internal/Magento/Framework/View/Layout/GeneratorPool.php

orlangur · 2019-04-17T09:22:50Z

lib/internal/Magento/Framework/View/Layout/GeneratorPool.php

     */
    public function process(Reader\Context $readerContext, Generator\Context $generatorContext)
    {
        $this->buildStructure($readerContext->getScheduledStructure(), $generatorContext->getStructure());
        foreach ($this->generators as $generator) {
+            $this->validateGenerator($generator);


Please validate in addGenerators only and get rid of this method.

@orlangur I believe this validation should be there too since the argument $generators can be injected via DI as well as using addGenerator method. Please let me know what you think about it.

How? As I see https://github.com/magento/magento2/blob/9c4e1758640ef6be4458929fc61d2619a78d9fc4/lib/internal/Magento/Framework/View/Layout/GeneratorPool.php#L51 it is still added via addGenerators this way.

Forget what I've said. It really doesn't make sense and I just made the commit.

orlangur · 2019-04-17T09:23:53Z

lib/internal/Magento/Framework/View/Layout/GeneratorPool.php

+    private function validateGenerator($generator)
+    {
+        if (!($generator instanceof GeneratorInterface)) {
+            throw new LocalizedException(__('Generator classes must be instances of %1', GeneratorInterface::class));


Do we really use LocalizedException in such cases? Please grep for some similar check, I believe it should be InvalidArgumentException and no need to translate.

orlangur

@tiagosampaio and now please inline the method as we need this check only once 😉

orlangur

After changes are applied and all builds are green, please squash them into a single commit so that we have perfectly clean history 😉

tiagosampaio · 2019-04-19T14:28:58Z

@orlangur it's done.

orlangur · 2019-04-19T14:31:01Z

lib/internal/Magento/Framework/View/Layout/GeneratorPool.php

     * @return void
     */
    protected function addGenerators(array $generators)
    {
        foreach ($generators as $generator) {
+            if (!($generator instanceof GeneratorInterface)) {


Please get rid of unnecessary parentheses.

magento-engcom-team · 2019-04-19T15:14:35Z

Hi @orlangur, thank you for the review.
ENGCOM-4750 has been created to process this Pull Request

VasylShvorak · 2019-04-23T13:15:57Z

Hi @tiagosampaio during testing PR changes I got fatal error instead of exception error.

But I can get exception error if I add message to InvalidArgumentException as string:
throw new \InvalidArgumentException( 'Generator class must be an instance of '. GeneratorInterface::class );

Could you take a look?

orlangur · 2019-04-23T13:25:11Z

lib/internal/Magento/Framework/View/Layout/GeneratorPool.php

     * @return void
     */
    protected function addGenerators(array $generators)
    {
        foreach ($generators as $generator) {
+            if (!$generator instanceof GeneratorInterface) {
+                throw new \InvalidArgumentException(
+                    'Generator class must be an instance of %1', GeneratorInterface::class


Yep, better use concatenation here (looks like you intended to use sprintf).

Commit amend + force push, please.

@orlangur I'm just using sprintf() now.

magento-engcom-team · 2019-04-23T22:54:39Z

Hi @orlangur, thank you for the review.
ENGCOM-4750 has been created to process this Pull Request

VasylShvorak · 2019-04-24T07:50:40Z

✔️ QA passed

m2-assistant · 2019-04-26T03:39:51Z

Hi @tiagosampaio, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

…nerator class. #22324

magento-engcom-team added Component: View Release Line: 2.3 labels Apr 13, 2019

dmytro-ch self-requested a review April 13, 2019 17:57

dmytro-ch self-assigned this Apr 13, 2019

dmytro-ch requested changes Apr 13, 2019

View reviewed changes

lib/internal/Magento/Framework/View/Layout/GeneratorPool.php Outdated Show resolved Hide resolved

ghost added the Progress: needs update label Apr 13, 2019

dmytro-ch approved these changes Apr 14, 2019

View reviewed changes

ghost added Progress: ready for testing and removed Progress: needs update labels Apr 14, 2019

orlangur requested changes Apr 14, 2019

View reviewed changes

ghost assigned orlangur Apr 14, 2019

ghost added Progress: needs update and removed Progress: ready for testing labels Apr 14, 2019

orlangur requested changes Apr 17, 2019

View reviewed changes

orlangur requested changes Apr 19, 2019

View reviewed changes

orlangur approved these changes Apr 19, 2019

View reviewed changes

ghost added Progress: ready for testing and removed Progress: needs update labels Apr 19, 2019

VasylShvorak self-assigned this Apr 23, 2019

ghost added Progress: testing in progress and removed Progress: ready for testing labels Apr 23, 2019

ghost added Progress: needs update and removed Progress: testing in progress labels Apr 23, 2019

orlangur requested changes Apr 23, 2019

View reviewed changes

Adding a validation before adding or executing layout generator class.

90eff3a

orlangur approved these changes Apr 23, 2019

View reviewed changes

ghost added Progress: ready for testing and removed Progress: needs update labels Apr 23, 2019

ghost added Progress: testing in progress and removed Progress: ready for testing labels Apr 24, 2019

ghost added Progress: extended testing and removed Progress: testing in progress labels Apr 24, 2019

nmalevanec assigned Nazar65 Apr 24, 2019

static test fix

a27c3e0

ghost added Progress: accept and removed Progress: extended testing labels Apr 24, 2019

magento-engcom-team merged commit a27c3e0 into magento:2.3-develop Apr 26, 2019

magento-engcom-team pushed a commit that referenced this pull request Apr 26, 2019

ENGCOM-4750: Adding a validation before adding or executing layout ge…

2da1c1f

…nerator class. #22324

magento-engcom-team added this to the Release: 2.3.2 milestone Apr 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding a validation before adding or executing layout generator class. #22324

Adding a validation before adding or executing layout generator class. #22324

tiagosampaio commented Apr 13, 2019 •

edited

Loading

m2-assistant bot commented Apr 13, 2019

magento-engcom-team commented Apr 14, 2019

orlangur Apr 14, 2019

tiagosampaio Apr 15, 2019

orlangur Apr 15, 2019

tiagosampaio Apr 16, 2019

orlangur Apr 17, 2019

tiagosampaio Apr 18, 2019 •

edited

Loading

orlangur Apr 18, 2019

tiagosampaio Apr 18, 2019

orlangur Apr 17, 2019

orlangur left a comment

orlangur left a comment

tiagosampaio commented Apr 19, 2019

orlangur Apr 19, 2019

tiagosampaio Apr 19, 2019

magento-engcom-team commented Apr 19, 2019

VasylShvorak commented Apr 23, 2019

orlangur Apr 23, 2019

tiagosampaio Apr 23, 2019

magento-engcom-team commented Apr 23, 2019

VasylShvorak commented Apr 24, 2019

m2-assistant bot commented Apr 26, 2019

Adding a validation before adding or executing layout generator class. #22324

Adding a validation before adding or executing layout generator class. #22324

Conversation

tiagosampaio commented Apr 13, 2019 • edited Loading

Fixed Issues (if relevant)

Contribution checklist (*)

m2-assistant bot commented Apr 13, 2019

magento-engcom-team commented Apr 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiagosampaio Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orlangur left a comment

Choose a reason for hiding this comment

orlangur left a comment

Choose a reason for hiding this comment

tiagosampaio commented Apr 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magento-engcom-team commented Apr 19, 2019

VasylShvorak commented Apr 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magento-engcom-team commented Apr 23, 2019

VasylShvorak commented Apr 24, 2019

m2-assistant bot commented Apr 26, 2019

tiagosampaio commented Apr 13, 2019 •

edited

Loading

tiagosampaio Apr 18, 2019 •

edited

Loading