Skip to content

AC-662: Create phpcs static check for AutogeneratedClassNotInConstructorTest #273

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 8 commits into from
Sep 23, 2021
200 changes: 200 additions & 0 deletions Magento2/Sniffs/PHP/AutogeneratedClassNotInConstructorSniff.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,200 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types = 1);

namespace Magento2\Sniffs\PHP;

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

/**
* Detects possible usage of 'var' language construction.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment accurate?

*/
class AutogeneratedClassNotInConstructorSniff implements Sniff
{
private const ERROR_CODE = 'AUTOGENERATED_CLASS_NOT_IN_CONSTRUCTOR';

/**
* @var array
*/
private $constructorParameters = [];

/**
* @var array
*/
private $uses = [];

/**
* @inheritdoc
*/
public function register()
{
return [T_FUNCTION, T_DOUBLE_COLON, T_USE];
}

/**
* @inheritdoc
*/
public function process(File $phpcsFile, $stackPtr)
{
if ($phpcsFile->getTokens()[$stackPtr]['type'] === 'T_USE') {
$this->registerUses($phpcsFile, $stackPtr);
}
if ($phpcsFile->getTokens()[$stackPtr]['type'] === 'T_FUNCTION') {
$this->registerConstructorParameters($phpcsFile, $stackPtr);
}
if ($phpcsFile->getTokens()[$stackPtr]['type'] === 'T_DOUBLE_COLON') {
if (!$this->isObjectManagerGetInstance($phpcsFile, $stackPtr)) {
return;
}

$statementStart = $phpcsFile->findStartOfStatement($stackPtr);
$statementEnd = $phpcsFile->findEndOfStatement($stackPtr);
$equalsPtr = $phpcsFile->findNext(T_EQUAL, $statementStart, $statementEnd);

if (!$equalsPtr) {
return;
}

$variableInParameters = false;
if ($variable = $phpcsFile->findNext(T_VARIABLE, $equalsPtr, $statementEnd)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is too long, please consider decomposition into several private functions

$variableName = $phpcsFile->getTokens()[$variable]['content'];
if ($variableName === '$this') {
$variableName = $this->getNext($phpcsFile, $variable, $statementEnd, T_STRING)['content'];
}
foreach ($this->constructorParameters as $parameter) {
$parameterName = $parameter['name'];
if ($this->variableName($parameterName) === $this->variableName($variableName)) {
$variableInParameters = true;
}
}
}

if (!$variableInParameters) {
$next = $stackPtr;
while ($next = $phpcsFile->findNext(T_DOUBLE_COLON, $next + 1, $statementEnd)) {
if ($this->getNext($phpcsFile, $next, $statementEnd, T_STRING)['content'] === 'class') {
$className = $this->getPrevious($phpcsFile, $next, T_STRING)['content'];
}
}

$className = $this->getClassNamespace($className);

$phpcsFile->addWarning(
sprintf("Class %s needs to be requested in constructor, " .
"otherwise compiler will not be able to find and generate these classes", $className),
$stackPtr,
self::ERROR_CODE
);
}
}
}

/**
* Check if it is a ObjectManager::getInstance
*
* @param File $phpcsFile
* @param int $stackPtr
* @return bool
*/
private function isObjectManagerGetInstance(File $phpcsFile, int $stackPtr): bool
{
$prev = $phpcsFile->findPrevious(T_STRING, $stackPtr - 1);
$next = $phpcsFile->findNext(T_STRING, $stackPtr + 1);
if ($prev &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified to

        return $prev &&
               $next &&
               $phpcsFile->getTokens()[$prev]['content'] === 'ObjectManager' &&
               $phpcsFile->getTokens()[$next]['content'] === 'getInstance';

$next &&
$phpcsFile->getTokens()[$prev]['content'] === 'ObjectManager' &&
$phpcsFile->getTokens()[$next]['content'] === 'getInstance') {
return true;
}
return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$prev = $phpcsFile->findPrevious(T_STRING, $stackPtr - 1);
$next = $phpcsFile->findNext(T_STRING, $stackPtr + 1);
if ($prev &&
$next &&
$phpcsFile->getTokens()[$prev]['content'] === 'ObjectManager' &&
$phpcsFile->getTokens()[$next]['content'] === 'getInstance') {
return true;
}
return false;
return $phpcsFile->findPrevious(T_STRING, $stackPtr - 1) &&
$phpcsFile->findNext(T_STRING, $stackPtr + 1) &&
$phpcsFile->getTokens()[$prev]['content'] === 'ObjectManager' &&
$phpcsFile->getTokens()[$next]['content'] === 'getInstance';

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code does not work, you are using variables $prev and $next that are not defined.

}

/**
* Get the complete class namespace from the use's
*
* @param string $className
* @return string
*/
private function getClassNamespace(string $className): string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this function handling alias like use Magento\Module\Class as MagentoModuleClass;?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not included.

{
foreach ($this->uses as $use) {
if (end($use) === $className) {
$className = implode('/', $use);
}
}
return $className;
}

/**
* Register php uses
*
* @param File $phpcsFile
* @param int $stackPtr
*/
private function registerUses(File $phpcsFile, int $stackPtr): void
{
$useEnd = $phpcsFile->findEndOfStatement($stackPtr);
$use = [];
$usePosition = $stackPtr;
while ($usePosition = $phpcsFile->findNext(T_STRING, $usePosition + 1, $useEnd)) {
$use[] = $phpcsFile->getTokens()[$usePosition]['content'];
}
$this->uses [] = $use;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be easier to process use in getClassNamespace if uses will be stored this way

Suggested change
$this->uses [] = $use;
$this->uses[end($use)] = implode('\', $use);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This option is easier to compare on line 122.

}

/**
* Register php constructor parameters
*
* @param File $phpcsFile
* @param int $stackPtr
*/
private function registerConstructorParameters(File $phpcsFile, int $stackPtr): void
{
$functionName = $phpcsFile->getDeclarationName($stackPtr);
if ($functionName == '__construct') {
$this->constructorParameters = $phpcsFile->getMethodParameters($stackPtr);
}
}

/**
* Get next token
*
* @param File $phpcsFile
* @param int $from
* @param int $to
* @param int|string|array $types
* @return mixed
*/
private function getNext(File $phpcsFile, int $from, int $to, $types)
{
return $phpcsFile->getTokens()[$phpcsFile->findNext($types, $from + 1, $to)];
}

/**
* Get previous token
*
* @param File $phpcsFile
* @param int $from
* @param int|string|array $types
* @return mixed
*/
private function getPrevious(File $phpcsFile, int $from, $types)
{
return $phpcsFile->getTokens()[$phpcsFile->findPrevious($types, $from - 1)];
}

/**
* Get name of the variable without $
*
* @param string $parameterName
* @return string
*/
protected function variableName(string $parameterName): string
{
return str_replace('$', '', $parameterName);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types = 1);

namespace Magento2\Tests\PHP;

use Magento2\Model;

class Good
{
public function __construct(
Model $model = null
)
{
$this->model = $model ?? ObjectManager::getInstance()->get(Model::class);
}

/**
* @return Model
*/
public function otherMethodThatCallsGetInstanceBad(): void
{
$model = ObjectManager::getInstance()->get(Model::class);
$model->something();
}

/**
* @return Model
*/
public function otherMethodThatCallsGetInstanceGood(): void
{
$model = $this->model ?? ObjectManager::getInstance()->get(Model::class);
$model->something();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is another case missing here, what about assigning an object manager instance to a variable?

Suggested change
}
}
/**
* @return Model
*/
public function objectManagerAssignedToAVariableBad(): void
{
$objectManager = ObjectManager::getInstance();
$model = $objectManager->get(Model::class);
$model->something();
}

Copy link
Contributor Author

@jcuerdo jcuerdo Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not on the ticket. And I can see in the original sniff is not covered I guess.

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types = 1);

namespace Magento2\Tests\PHP;

class Bad
{
public function __construct()
{
$this->model = ObjectManager::getInstance()->get(Model::class);
}
}

39 changes: 39 additions & 0 deletions Magento2/Tests/PHP/AutogeneratedClassNotInConstructorUnitTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types = 1);

namespace Magento2\Tests\PHP;

use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest;

class AutogeneratedClassNotInConstructorUnitTest extends AbstractSniffUnitTest
{
/**
* @inheritdoc
*/
public function getWarningList($filename = '')
{
if ($filename === 'AutogeneratedClassNotInConstructorUnitTest.1.php.inc') {
return [
26 => 1,
];
}
if ($filename === 'AutogeneratedClassNotInConstructorUnitTest.2.php.inc') {
return [
14 => 1,
];
}
return [];
}

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