Skip to content

Add ExtND rules to EcgM2 ruleset. #62

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 18, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions EcgM2/Samples/Blocks/SetTemplateInBlock.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?php
namespace EcgM2\Samples\Blocks;

class SetTemplateInBlock
{
public function __construct()
{
$this->setTemplate('foobar.phtml');
}
}
10 changes: 10 additions & 0 deletions EcgM2/Samples/Classes/StrictTypes.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?php

declare(strict_types=0);

namespace EcgM2\Samples\Classes;

class StrictTypes
{

}
3 changes: 3 additions & 0 deletions EcgM2/Samples/Templates/objectmanager.phtml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<div>
<?php $objectManager = \Magento\Framework\App\ObjectManager::getInstance(); ?>
</div>
28 changes: 28 additions & 0 deletions EcgM2/Sniffs/Blocks/SetTemplateInBlockSniff.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# Rule: Do not use setTemplate in Block classes
## Background
When creating a Block class, a Block class could set its PHTML template in multiple ways: Through XML layout, through a
call to `$this->setTemplate()` and through a variable `$_template`. The new design of Block classes is to configure them
at constructor time, meaning that configuration options (like the template) are added using constructor arguments. This
allows for the XML layout to change the template. The template in the Block class is then only defined as a default value,
if the XML layout is not overriding the template: This default value is best defined via a protected variable
`$_template`.

## Reasoning

If `$this->setTemplate()` is used instead, this could lead to potential issues: First of all, setters are deprecated in
Block classes (because constructor arguments should be preferred instead). Second, if `$this->setTemplate()` is added to
the constructor *after* calling upon the parent constructor, it would undo the configuration via XML layout. Simply put:
It is outdated and leads to issues quickly.

## How it works
This rule checks whether a Block class uses `$this->setTemplate()`.

## How to fix

If there is a match, the preferred way of optimizing your code would be as follows:

class MyBlock extends Template
{
protected $_template = 'foobar.phtml';
}

57 changes: 57 additions & 0 deletions EcgM2/Sniffs/Blocks/SetTemplateInBlockSniff.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
<?php

declare(strict_types=1);

namespace EcgM2\Sniffs\Blocks;

use PHP_CodeSniffer\Sniffs\Sniff;
use PHP_CodeSniffer\Files\File;

class SetTemplateInBlockSniff implements Sniff
{
/**
* @var string
*/
protected $message = 'Define $_template instead of using $this->setTemplate() in Block classes.';

/**
* @var array
*/
public $supportedTokenizers = ['PHP'];

/**
* @return array|int[]
*/
public function register()
{
return [T_CLASS];
}

/**
* @param File $phpcsFile
* @param int $stackPtr
* @return int|void
*/
public function process(File $phpcsFile, $stackPtr)
{
$tokens = $phpcsFile->getTokens();

foreach ($tokens as $line => $token) {
if ($this->hasTokenMatch($token) === false) {
continue;
}

$error = $this->message . ' Found: %s';
$phpcsFile->addWarning($error, $line, 'Found', $token);
}
}

private function hasTokenMatch(array $token): bool
{
if ($token['content'] !== 'setTemplate') {
return false;
}

return true;
}
}
19 changes: 19 additions & 0 deletions EcgM2/Sniffs/Classes/StrictTypesSniff.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# Rule: Add `declare(strict_types=1)` to your PHP files
## Background
With PHP 7, it is possible to add type hinting to your code. However, this doesn't mean that types are actually enforced, unless strict typing is
enabled by adding `declare(strict_types=1)` to the top of each PHP file.

## Reasoning
PHP code becomes more robust when type hinting (argument types, return types) are added. With the `declare(strict_types=1)` added, there is less
chance for bugs that related to type casting.

## How it works
This rule scans the source code to see whether a line `declare(strict_type=1)` occurs or not.

## How to fix
Simply add a statement `declare(strict_types=1)` to the top of your files:

<?php
declare(strict_types=1);

namespace Foo\Bar;
79 changes: 79 additions & 0 deletions EcgM2/Sniffs/Classes/StrictTypesSniff.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
<?php

declare(strict_types=1);

namespace EcgM2\Sniffs\Classes;

use PHP_CodeSniffer\Files\File;
use PHP_CodeSniffer\Sniffs\Sniff;

class StrictTypesSniff implements Sniff
{
/**
* @var array
*/
public $supportedTokenizers = ['PHP'];

/**
* @var string
*/
protected $message = 'Define declare(strict_types=1).';

/**
* @return array|int[]
*/
public function register()
{
return [T_CLASS];
}

/**
* @param File $phpcsFile
* @param int $stackPtr
* @return int|void
*/
public function process(File $phpcsFile, $stackPtr)
{
$tokens = $phpcsFile->getTokens();
$hasDeclareStrictTypes = false;

$declarePosition = $phpcsFile->findNext([T_DECLARE], 0);
if ($declarePosition !== false) {
$lineEnd = $phpcsFile->findNext([T_SEMICOLON], $declarePosition);

// extract all tokens between declare and line end
$tokens = \array_slice($tokens, $declarePosition, $lineEnd - 1);
$hasDeclareStrictTypes = $this->hasDeclareStrictTypes($tokens);
}

if ($hasDeclareStrictTypes === false) {
$error = $this->message . ' Not Found';
$phpcsFile->addWarning($error, null, 'NotFound');
}
}

/**
* Checks has the File with strict_types=1
*
* @param array $tokens
* @param $start
* @return bool
*/
private function hasDeclareStrictTypes(array $tokens): bool
{
$containsStrictType = false;
$isEnabled = false;

foreach ($tokens as $token) {
if ($token['code'] === T_STRING && $token['content'] === 'strict_types') {
$containsStrictType = true;
}

if ($token['code'] === T_LNUMBER && $token['content'] === '1') {
$isEnabled = true;
}
}

return $containsStrictType && $isEnabled;
}
}
104 changes: 104 additions & 0 deletions EcgM2/Sniffs/Templates/TemplateObjectManagerSniff.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
# Rule: Do not use the object manager in templates
## Background
The Object Manager is responsible to create concrete objects for a required interface, and to generate code e.g. for adding plugins, factories, proxies.
It is used transparently by requesting a class/interface name as constructor dependency or in layout XML.

You can use it directly with `ObjectManager::getInstance()->get(SomeInterface::class)` and `ObjectManager::getInstance()->create(SomeInterface::class)`, but this is not encouraged.

## Reasoning
Magento offers an extensive system for dependency injection which allows us to access and create objects. It also makes it
very clear, which classes or interfaces are needed by a particular piece of code and forces the developer to re-evaluate
the design if there are too many.

This dependency injection system uses the object manager, but it is an implementation detail that should stay hidden.
Bypassing this system leads to hidden dependencies and potentially to too many of them.

Besides, for separation of appearance and logic, a template should contain as little and uncomplex PHP code as possible.

## How it works
For all PHTML files, the sniff looks for string tokens "ObjectManager".

## How to fix
Given, your template contains code like this:

```php
<?php
$choo = \Magento\Framework\App\ObjectManager::getInstance()->get(\Choo\Choo::class);
```

1. if the template is accompanied by a custom block, you can add the dependency to the block:

```php
<?php
namespace Extdn\Example;

use Magento\Framework\View\Element\Template;

class TheBlock extends Template
{
/** @var Choo\Choo */
private $choo;

public function __construct(Context $context, Choo\Choo $choo)
{
$this->choo = $choo;
}

public function getChoo(): Choo\Choo
{
return $this->choo;
}

}
```

and change the template to

```php
<?php
$choo = $block->getChoo();
```
2. Starting with Magento 2.2, **view models** are considered a better alternative to blocks. Add a view model, or use an existing one,
and add the dependency there:

```php
<?php
namespace Extdn\Example;

use Magento\Framework\View\Element\Block\ArgumentInterface;

class TheViewModel implements ArgumentInterface
{
/** @var Choo\Choo */
private $choo;

public function __construct(Choo\Choo $choo)
{
$this->choo = $choo;
}

public function getChoo(): Choo\Choo
{
return $this->choo;
}

}
```

The view model is added to the block via layout XML:

```xml
<block name="my.block" template="Extdn_Example::template.phtml" >
<arguments>
<argument name="the_view_model" xsi:type="object">Extdn\Example\TheViewModel</argument>
</arguments>
</block>
```

and can be used in the template like this:

```php
<?php
$theViewModel = $block->getData('the_view_model');
$choo = $theViewModel->getChoo();
```
60 changes: 60 additions & 0 deletions EcgM2/Sniffs/Templates/TemplateObjectManagerSniff.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
<?php

declare(strict_types=1);

namespace EcgM2\Sniffs\Templates;

use PHP_CodeSniffer\Files\File;
use PHP_CodeSniffer\Sniffs\Sniff;

class TemplateObjectManagerSniff implements Sniff
{


public $templateExtensionList = ['.phtml'];
/**
* @var string
*/
protected $message = 'Define dependencies in block or view model instead of using ObjectManager in the template.';

/**
* @inheritdoc
*/
public function register()
{
return [T_OPEN_TAG];
}

/**
* {@inheritdoc}
*/
public function process(File $phpcsFile, $stackPtr)
{
$tokens = $phpcsFile->getTokens();
$fileName = $phpcsFile->getFileName();
$extension = substr($fileName, strrpos($fileName, '.'));

if (\in_array($extension, $this->templateExtensionList, false) === false) {
return;
}

foreach ($tokens as $line => $token) {
if ($this->hasTokenMatch($token) === false) {
continue;
}

$error = $this->message . ' Found: %s';
$phpcsFile->addError($error, $line, 'Found', $token);
}
}


public function hasTokenMatch($token)
{
if ($token['content'] !== 'ObjectManager') {
return false;
}

return true;
}
}
10 changes: 10 additions & 0 deletions EcgM2/Tests/Blocks/SetTemplateInBlockUnitTest.inc
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?php
namespace EcgM2\Samples\Blocks;

class SetTemplateInBlock
{
public function __construct()
{
$this->setTemplate('foobar.phtml');
}
}
Loading