Merge pull request #300 from shochdoerfer/feature/rule_disallow_setTe…

…mplate

Add rule "Disallow setTemplate() in Block classes"

docs/features.md

@@ -83,3 +83,14 @@ parameters:
     magento:
         checkCollectionViaFactory: false
 ```
+
+### Do not use setTemplate in Block classes
+
+As the [ExtDN](https://github.com/extdn/extdn-phpcs/blob/master/Extdn/Sniffs/Blocks/SetTemplateInBlockSniff.md) folks have put it: Setters are deprecated in Block classes, because constructor arguments should be preferred instead. Any call to `$this->setTemplate()` after calling upon the parent constructor would undo the configuration via XML layout.
+
+To disable this rule add the following code to your `phpstan.neon` configuration file:
+```neon
+parameters:
+    extdn:
+        setTemplateDisallowedForBlockClasses: false
+```

extension.neon

@@ -4,6 +4,8 @@ parameters:
 		checkServiceContracts: true
 		checkResourceModelsUsedDirectly: true
 		magentoRoot: %currentWorkingDirectory%
+	extdn:
+		setTemplateDisallowedForBlockClasses: true
 	bootstrapFiles:
 		- magento-autoloader.php
 parametersSchema:
@@ -20,6 +22,8 @@ conditionalTags:
 		phpstan.rules.rule: %magento.checkServiceContracts%
 	bitExpert\PHPStan\Magento\Rules\ResourceModelsShouldBeUsedDirectlyRule:
 		phpstan.rules.rule: %magento.checkResourceModelsUsedDirectly%
+	bitExpert\PHPStan\Magento\Rules\SetTemplateDisallowedForBlockRule:
+		phpstan.rules.rule: %extdn.setTemplateDisallowedForBlockClasses%
 services:
 	-
 		class: bitExpert\PHPStan\Magento\Type\ObjectManagerDynamicReturnTypeExtension
@@ -47,6 +51,8 @@ services:
 		class: bitExpert\PHPStan\Magento\Rules\AbstractModelUseServiceContractRule
 	-
 		class: bitExpert\PHPStan\Magento\Rules\ResourceModelsShouldBeUsedDirectlyRule
+	-
+		class: bitExpert\PHPStan\Magento\Rules\SetTemplateDisallowedForBlockRule
 	fileCacheStorage:
 		class: bitExpert\PHPStan\Magento\Autoload\Cache\FileCacheStorage
 		arguments:

phpstan.neon

@@ -52,6 +52,9 @@ parameters:
         -
             message: '~bitExpert\\PHPStan\\Magento\\Rules\\Helper\\SampleModel::__construct\(\) does not call parent constructor~'
             path: tests/bitExpert/PHPStan/Magento/Rules/Helper/SampleModel.php
+        -
+            message: '~bitExpert\\PHPStan\\Magento\\Rules\\Helper\\SampleBlock::__construct\(\) does not call parent constructor~'
+            path: tests/bitExpert/PHPStan/Magento/Rules/Helper/SampleBlock.php
         -
             message: '~Call to static method PHPUnit\\Framework\\Assert::assertInstanceOf~'
             path: tests/bitExpert/PHPStan/Magento/Type/TestFrameworkObjectManagerDynamicReturnTypeExtensionUnitTest.php

src/bitExpert/PHPStan/Magento/Rules/SetTemplateDisallowedForBlockRule.php

@@ -0,0 +1,74 @@
+<?php
+
+/*
+ * This file is part of the phpstan-magento package.
+ *
+ * (c) bitExpert AG
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+declare(strict_types=1);
+
+namespace bitExpert\PHPStan\Magento\Rules;
+
+use PhpParser\Node;
+use PhpParser\Node\Expr\MethodCall;
+use PHPStan\Analyser\Scope;
+use PHPStan\Rules\Rule;
+use PHPStan\ShouldNotHappenException;
+use PHPStan\Type\ObjectType;
+use PHPStan\Type\VerbosityLevel;
+
+/**
+ * Do not use setTemplate in Block classes, see
+ * https://github.com/extdn/extdn-phpcs/blob/master/Extdn/Sniffs/Blocks/SetTemplateInBlockSniff.md
+ *
+ * @implements Rule<MethodCall>
+ */
+class SetTemplateDisallowedForBlockRule implements Rule
+{
+    /**
+     * @phpstan-return class-string<MethodCall>
+     * @return string
+     */
+    public function getNodeType(): string
+    {
+        return MethodCall::class;
+    }
+
+    /**
+     * @param Node $node
+     * @param Scope $scope
+     * @return (string|\PHPStan\Rules\RuleError)[] errors
+     * @throws ShouldNotHappenException
+     */
+    public function processNode(Node $node, Scope $scope): array
+    {
+        if (!$node instanceof MethodCall) {
+            throw new ShouldNotHappenException();
+        }
+
+        if (!$node->name instanceof Node\Identifier) {
+            return [];
+        }
+
+        if ($node->name->name !== 'setTemplate') {
+            return [];
+        }
+
+        $type = $scope->getType($node->var);
+        $isAbstractModelType = (new ObjectType('Magento\Framework\View\Element\Template'))->isSuperTypeOf($type);
+        if (!$isAbstractModelType->yes()) {
+            return [];
+        }
+
+        return [
+            sprintf(
+                'Setter methods like %s::%s() are deprecated in Block classes, use constructor arguments instead',
+                $type->describe(VerbosityLevel::typeOnly()),
+                $node->name->name
+            )
+        ];
+    }
+}

tests/bitExpert/PHPStan/Magento/Rules/Helper/SampleBlock.php

@@ -0,0 +1,24 @@
+<?php
+
+/*
+ * This file is part of the phpstan-magento package.
+ *
+ * (c) bitExpert AG
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+declare(strict_types=1);
+
+namespace bitExpert\PHPStan\Magento\Rules\Helper;
+
+use \Magento\Framework\View\Element\Template;
+
+class SampleBlock extends Template
+{
+    public function __construct()
+    {
+        // We do not call the parent constructor here on purpose as we do not want do to deal with creating
+        // not needed dependencies just for the testcase!
+    }
+}

tests/bitExpert/PHPStan/Magento/Rules/Helper/block_settemplate.php

@@ -0,0 +1,4 @@
+<?php
+
+$block = new \bitExpert\PHPStan\Magento\Rules\Helper\SampleBlock();
+$block->setTemplate('template.phtml');

tests/bitExpert/PHPStan/Magento/Rules/SetTemplateDisallowedForBlockRuleUnitTest.php

@@ -0,0 +1,99 @@
+<?php
+
+/*
+ * This file is part of the phpstan-magento package.
+ *
+ * (c) bitExpert AG
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+declare(strict_types=1);
+
+namespace bitExpert\PHPStan\Magento\Rules;
+
+use bitExpert\PHPStan\Magento\Rules\Helper\SampleBlock;
+use bitExpert\PHPStan\Magento\Rules\Helper\SampleModel;
+use PhpParser\Node\Expr\MethodCall;
+use PhpParser\Node\Expr\Variable;
+use PHPStan\Analyser\Scope;
+use PHPStan\Rules\Rule;
+use PHPStan\ShouldNotHappenException;
+use PHPStan\Testing\RuleTestCase;
+
+/**
+ * @extends \PHPStan\Testing\RuleTestCase<SetTemplateDisallowedForBlockRule>
+ */
+class SetTemplateDisallowedForBlockRuleUnitTest extends RuleTestCase
+{
+    protected function getRule(): Rule
+    {
+        return new SetTemplateDisallowedForBlockRule();
+    }
+
+    /**
+     * @test
+     */
+    public function checkCaughtExceptions(): void
+    {
+        $this->analyse([__DIR__ . '/Helper/block_settemplate.php'], [
+            [
+                'Setter methods like '.SampleBlock::class.'::setTemplate() are deprecated in Block classes, '.
+                'use constructor arguments instead',
+                4,
+            ],
+        ]);
+    }
+
+    /**
+     * @test
+     */
+    public function getNodeTypeMethodReturnsMethodCall(): void
+    {
+        $rule = new SetTemplateDisallowedForBlockRule();
+
+        self::assertSame(MethodCall::class, $rule->getNodeType());
+    }
+
+    /**
+     * @test
+     */
+    public function processNodeThrowsExceptionForNonMethodCallNodes(): void
+    {
+        $this->expectException(ShouldNotHappenException::class);
+
+        $node = new Variable('var');
+        $scope = $this->createMock(Scope::class);
+
+        $rule = new SetTemplateDisallowedForBlockRule();
+        $rule->processNode($node, $scope);
+    }
+
+    /**
+     * @test
+     */
+    public function processNodeReturnsEarlyWhenNodeNameIsWrongType(): void
+    {
+        $node = new MethodCall(new Variable('var'), new Variable('wrong_node'));
+        $scope = $this->createMock(Scope::class);
+
+        $rule = new SetTemplateDisallowedForBlockRule();
+        $return = $rule->processNode($node, $scope);
+
+        self::assertCount(0, $return);
+    }
+
+    /**
+     * @test
+     */
+    public function processNodeReturnsEarlyWhenNodeNameIsNotSaveOrLoadOrDelete(): void
+    {
+        $node = new MethodCall(new Variable('var'), 'wrong_node_name');
+        $scope = $this->createMock(Scope::class);
+
+        $rule = new SetTemplateDisallowedForBlockRule();
+        $return = $rule->processNode($node, $scope);
+
+        self::assertCount(0, $return);
+    }
+}