Skip to content

Created Sniff to validate function without argument #325

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

Conversation

anzin
Copy link
Contributor

@anzin anzin commented Nov 5, 2021

Description (*)

Added sniff for check deprecated function without argument.

Addresses backward incompatible change in PHP 8.1: https://wiki.php.net/rfc/deprecations_php_8_1

Fixed Issues (if relevant)

  1. Add new sniff Magento2.Functions.DeprecatedFunction into magento-coding-standard magento2#34547

Contribution checklist (*)

  • Author has signed the Adobe CLA
  • 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 are green)

@anzin anzin force-pushed the improvements/sniff-to-check-php-deprecated-function branch from 2778553 to 1a9b4a4 Compare November 5, 2021 10:37
andrewbess
andrewbess previously approved these changes Nov 5, 2021
Copy link
Member

@sivaschenko sivaschenko left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request @anzin , please see my comments

Comment on lines 30 to 36
if ($testFile === 'DeprecatedFunctionUnitTest.inc') {
return [
23 => 1
];
}

return [];
Copy link
Member

Choose a reason for hiding this comment

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

Can it be simplified considering there is only one test file?

Suggested change
if ($testFile === 'DeprecatedFunctionUnitTest.inc') {
return [
23 => 1
];
}
return [];
return [
23 => 1
];

* @var array
*/
private $deprecatedFunctions = [
'mb_check_encoding'
Copy link
Member

Choose a reason for hiding this comment

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

I believe the following functions should be added to the list:

  • get_class
  • get_parent_class
  • get_called_class

According to https://wiki.php.net/rfc/deprecations_php_8_1

/**
* Sniff to validate PHP deprecated function.
*/
class DeprecatedFunctionSniff implements Sniff
Copy link
Member

Choose a reason for hiding this comment

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

I think the following name would better describe the purpose of this sniff

Suggested change
class DeprecatedFunctionSniff implements Sniff
class FunctionsDeprecatedWithoutArgumentSniff implements Sniff

use PHP_CodeSniffer\Sniffs\Sniff;

/**
* Sniff to validate PHP deprecated function.
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
* Sniff to validate PHP deprecated function.
* Sniff to validate PHP functions usage of which without passing arguments is deprecated.


if (!$argumentString) {
$warningMessage = sprintf(
'Calling function %s() without argument is deprecated in PHP 8.1',
Copy link
Member

Choose a reason for hiding this comment

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

Can you please provide a suggestion on how to fix this issue in the message?

/**
* @inheritdoc
*/
public function process(File $phpcsFile, $stackPtr): void
Copy link
Member

@sivaschenko sivaschenko Nov 5, 2021

Choose a reason for hiding this comment

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

A good approach for phpcs tests (that also matches the design of this static testing library) is utilizing the iteration that is already done by the library instead of registering T_OPEN_TAG to process every file and performing an additional custom parsing/iteration of the file.

Please see an example of the implementation where each invoked relevant token is analyzed:

/**
     * Deprecated functions without argument.
     *
     * @var array
     */
    private $deprecatedFunctions = [
        'mb_check_encoding' => 'Calling function mb_check_encoding() without argument is deprecated in PHP 8.1. '
            . 'Please pass the input to check for encoding as the first argument of the function.'
    ];

    /**
     * @inheritdoc
     */
    public function register(): array
    {
        return [
            T_OPEN_PARENTHESIS
        ];
    }

    /**
     * @inheritdoc
     */
    public function process(File $phpcsFile, $stackPtr): void
    {
        $closeParenthesisPtr = $phpcsFile->findNext(T_CLOSE_PARENTHESIS, $stackPtr);
        $arguments = trim($phpcsFile->getTokensAsString($stackPtr + 1, $closeParenthesisPtr - $stackPtr - 1));
        if ($arguments) {
            return;
        }

        $functionName = $phpcsFile->getTokensAsString($phpcsFile->findPrevious(T_STRING, $stackPtr), 1);

        if (isset($this->deprecatedFunctions[$functionName])) {
            $phpcsFile->addWarning($this->deprecatedFunctions[$functionName], $stackPtr, 'FunctionDeprecatedWithoutArguments');
        }
    }

@anzin anzin force-pushed the improvements/sniff-to-check-php-deprecated-function branch from ac3333f to c895716 Compare November 5, 2021 13:54
@anzin anzin requested a review from sivaschenko November 5, 2021 13:55
@anzin
Copy link
Contributor Author

anzin commented Nov 5, 2021

Hello @sivaschenko , thanks for your review, I fixed code by you recommendation, please check if every think fine.

Copy link
Member

@sivaschenko sivaschenko left a comment

Choose a reason for hiding this comment

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

Thanks for the update @anzin ! The implementation looks great!

The final thing that I'd recommend to improve the sniff is using constants instead of variables for the values that are not supposed to be changed/updated in runtime.

*
* @var array
*/
private $deprecatedFunctions = [
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
private $deprecatedFunctions = [
private const FUNCTIONS_LIST = [

*
* @var string
*/
private $warningCode = 'FunctionsDeprecatedWithoutArgument';
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
private $warningCode = 'FunctionsDeprecatedWithoutArgument';
private const WARNING_CODE = 'FunctionsDeprecatedWithoutArgument';

*
* @var string
*/
private $warningMessage = 'Calling function %s() without argument is deprecated in PHP 8.1. '
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
private $warningMessage = 'Calling function %s() without argument is deprecated in PHP 8.1. '
private const WARNING_MESSAGE = 'Calling function %s() without argument is deprecated in PHP 8.1. '


$functionName = $phpcsFile->getTokensAsString($phpcsFile->findPrevious(T_STRING, $stackPtr), 1);

if (in_array($functionName, $this->deprecatedFunctions)) {
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
if (in_array($functionName, $this->deprecatedFunctions)) {
if (in_array($functionName, self::FUNCTIONS_LIST)) {

$functionName = $phpcsFile->getTokensAsString($phpcsFile->findPrevious(T_STRING, $stackPtr), 1);

if (in_array($functionName, $this->deprecatedFunctions)) {
$phpcsFile->addWarning(sprintf($this->warningMessage, $functionName), $stackPtr, $this->warningCode);
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
$phpcsFile->addWarning(sprintf($this->warningMessage, $functionName), $stackPtr, $this->warningCode);
$phpcsFile->addWarning(sprintf(self::WARNING_MESSAGE, $functionName), $stackPtr, self::WARNING_CODE);

@anzin anzin requested a review from sivaschenko November 7, 2021 16:39
@anzin
Copy link
Contributor Author

anzin commented Nov 9, 2021

Hello @sivaschenko, I fixed code by you recommendation, please check if every think fine.

@sivaschenko
Copy link
Member

@magento import pr to magento-commerce/magento-coding-standard

@m2-github-services
Copy link
Contributor

@sivaschenko an error occurred during the Pull Request import.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants