#121 Magento2.Templates.HelperInTemplate Rule porposal

[diazwatson]

#121

[lenaorobei]

@diazwatson, I got approval magento/architecture#222 that it is a valid recommendation and we can move forward with this rule.

However, I have few comments:

1. Consider moving this check to the ThisInTemplateSniff. You can ask why? 🙂 Those two checks register the same tokens and are designed for phtml files. If we keep them both we run quite similar check twice instead of once. If there are 10 files it is not a big deal, but for the whole app/code it takes too long. There can be a second code: 'FoundHelper in the \Magento2\Sniffs\Templates\ThisInTemplateSniff.

2. Make sure that for the following case only one warning will be raised (do not use helpers).

$this->helper();

3. Please add this case to the unit test \Magento2\Tests\Templates\ThisInTemplateUnitTest.

[lenaorobei]

In most cases it is $this->helper(<helper_class>), because $this refers to the template engine, not to the block.

[diazwatson]

Cool, I'll update this PR with those changes

[lenaorobei]

I checked the file with following content:

<?php
echo $block->escapeHtml($block->getGroupCode());
echo $this->escapeHtml($this->getGroupCode());
$this->foo();
$this->helper();

Actual result:

FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 3 LINES
----------------------------------------------------------------------------------------------------------
 3 | WARNING | Usage of $this in template files is deprecated.
 3 | WARNING | Usage of $this in template files is deprecated.
 4 | WARNING | Usage of $this in template files is deprecated.
 5 | WARNING | Usage of $this in template files is deprecated.
----------------------------------------------------------------------------------------------------------

Expected result:

FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 3 LINES
----------------------------------------------------------------------------------------------------------
 3 | WARNING | Usage of $this in template files is deprecated.
 3 | WARNING | Usage of $this in template files is deprecated.
 4 | WARNING | Usage of $this in template files is deprecated.
 5 | WARNING | Usage of helpers in templates is discouraged.
----------------------------------------------------------------------------------------------------------

[diazwatson]

Hi @lenaorobei could you share the steps or commands you are using for test?

[lenaorobei]

I just created .phtml file with following content:

<?php
echo $block->escapeHtml($block->getGroupCode());
echo $this->escapeHtml($this->getGroupCode());
$this->foo();
$this->helper();

Then I run command

vendor/bin/phpcs --standard=Magento2 --sniffs=Magento2.Templates.ThisInTemplate /path/to/this/file.phtml

[diazwatson]

Hi @lenaorobei I have updated this PR with the changes requested.

Some thoughts about these changes:

1. I still think that having a separate rule benefits more than it harms:

• it is easier to understand
• it is easier to maintain
• allows for isolated configuration of each rule

besides, some other sniffers have several rules,

Some examples are

• Magento2.PHP (6 rules)
• Magento2.Security (7 rules)

2. If the previous is not enough to have a separated rule and we still need put this rule inside ThisInTemplate sniff I think we should rename the rule.

The rule is called ThisInTemplate, this name clearly tells that the rule evaluates the use of $this in template files, but now the rule does more than that, and therefore the name feels a bit wrong/misleading.

What do you think about renaming the rule to something more generic?

Something like FoundInTemplate?

3. I think 2 warnings should be generated for line 5.

----------------------------------------------------------------------
FOUND 0 ERRORS AND 5 WARNINGS AFFECTING 3 LINES
----------------------------------------------------------------------
 3 | WARNING | Usage of $this in template files is deprecated.
 3 | WARNING | Usage of $this in template files is deprecated.
 4 | WARNING | Usage of $this in template files is deprecated.
 5 | WARNING | Usage of $this in template files is deprecated.
 5 | WARNING | Usage of helpers in templates is discouraged.
----------------------------------------------------------------------

[lenaorobei]

@diazwatson thanks for sharing your suggestions, I really appreciate it. However, I did some sync up with internal teams regarding this rule and we agreed following:

1. Let's keep one sniff for these checks. Separate sniff code is good enough to distinguish between findings. Sniff code can serve for configuration purposes as well.

2. I do understand that the name could be better, but unfortunately we cannot change the name because of BC. ThisInTemplate is widely used in the core codebase for ignoring lines of code. We would have to rewrite them all.
   https://github.com/magento/magento2/blob/165640945113373a775bf16620d8291320ca52cd/app/code/Magento/Catalog/view/frontend/templates/product/view/description.phtml#L7

3. The line $this->helper(); should produce one warning: Usage of helpers in templates is discouraged.. The second message about Usage of $this in template is not relevant here, because $block->helper(); wouldn't work. $block refers to the actual block and it does not have helper method. $this refers to the template engine and it magically calls helper.

[diazwatson]

Hi @lenaorobei could you explain how this should be then refactor to generate only one warning?

[lenaorobei]

It should be something like this:

    public function process(File $phpcsFile, $stackPtr)
    {
        $tokens = $phpcsFile->getTokens();
        if ($tokens[$stackPtr]['content'] === '$this') {
            $helperPosition = $phpcsFile->findNext(T_STRING, $stackPtr, null, false, 'helper', true);
            if ($helperPosition !== false) {
                $phpcsFile->addWarning($this->warningMessageFoundHelper, $helperPosition, $this->warningCodeFoundHelper);
            } else {
                $phpcsFile->addWarning($this->warningMessage, $stackPtr, $this->warningCode);
            }
        }
    }

[diazwatson]

@lenaorobei Thanks for the feedback, please have a look at the latest changes when you get a chance.

[lenaorobei]

We are looking for $this, so we need to subscribe to T_VARIABLE token only. T_STRING is redundant here.