PHPStan add support of magic methods of Data Object.

[swnsma]

• Add extension to support of 'get/set/has/uns' 'magic' methods (with usage of __call) for DataObject;
• Add support for SessionManager (it forwards all calls to DataObject container);
• Add test coverage for extensions;
• fix outdated tests for Filtered Error check;
• increase PHPStan check level 0 -> 1.

Q:

• Are tests from dev/tests/static/framework/tests folder also included to test builds?

Resolved issues:

1. resolves [Issue] PHPStan add support of magic methods of Data Object. #28303: PHPStan add support of magic methods of Data Object.

[m2-assistant]

Hi @swnsma. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

[viktym]

Hi @swnsma
Thank you for your contribution
Increasing the PHPStan level from 0 to 1 involves activation next additional rules and checks:

parameters:
	checkMaybeUndefinedVariables: true
	checkExtraArguments: true
	reportMagicMethods: true
	reportMagicProperties: true


conditionalTags:
	PHPStan\Rules\Variables\VariableCertaintyNullCoalesceRule:
		phpstan.rules.rule: %featureToggles.nullCoalesce%

rules:
	- PHPStan\Rules\Classes\UnusedConstructorParametersRule
	- PHPStan\Rules\Constants\ConstantRule
	- PHPStan\Rules\Functions\UnusedClosureUsesRule
	- PHPStan\Rules\Variables\VariableCertaintyInIssetRule

services:
	-
		class: PHPStan\Rules\Variables\VariableCertaintyNullCoalesceRule

Can you run PHPStan check using level 1 on CE/EE/B2B Magento codebase and compare results with level 0 (create a diff) ?
Please, make sure that all reported errors can be fixed when Magento developers will face them in their PRs. Or, possibly, some files should be added in blacklist at once.
Also, please check if new PHPStan checks intersect with PHPMD and PHPCS checks. We should avoid double suppression (like unused method argument etc.).

[swnsma]

Hi, @viktym!

Please find the diff: https://www.diffchecker.com/rHxwkJVM
In total there are 1604 vs 2356 errors.
I have used CE + MSI + EE + B2B codebase (lib + app/code + dev + setup).
From the diff I can sum up that for Magento relevant next new checks:

• incorrect number of arguments passed;
• a variable might not be defined check;
• isset usage;
• access to the undefined property;
• anonymous function and use and closure;
• call to the undefined method;
• find undefined constants;

From all these violations, I see problems with constants (most of the problems are related to tests).

1. Constants which is defined by tests settings:

• TESTS_WEB_API_ADAPTER
• TESTS_BASE_URL
• TESTS_XDEBUG_ENABLED
• TESTS_XDEBUG_SESSION
  As they did not declare in PHP directly, I suggest adding these constants to "ignoreErrors" in phpstan.neon

2. Constants defined by tests fixtures:

• STAGING_UPDATE_FIXTURE_ID
• FIXTURE_ATTRIBUTE_USER_DEFINED_CUSTOMER_NAME
• FIXTURE_ATTRIBUTE_USER_DEFINED_CUSTOMER_FRONTEND_LABEL
  I suggest to skip them and just use "phpstan:ignore"

3. Constants, defined by tests setup:

• INTEGRATION_TESTS_DIR
• MAGENTO_MODULES_PATH
• TESTS_MODULES_PATH
• TESTS_INSTALLATION_DB_CONFIG_FILE
  These constants are defined on the fly and PHPStan needs to have files with constants definition in autoload.
  We need to autoload: dev/tests/setup-integration/framework/bootstrap.php and dev/tests/api-functional/framework/bootstrap.php
  But I suggest also to add these constants to "ignoreErrors".

4. Constants defined by PHP CS

• T_STRING_CONCAT
• T_OPEN_CURLY_BRACKET
• T_CLOSE_CURLY_BRACKET
• T_OPEN_PARENTHESIS
• T_CLOSE_PARENTHESIS
• T_STYLE
• T_ASPERAND
• T_OPEN_CURLY_BRACKET
• T_CLOSE_CURLY_BRACKET
• T_OPEN_CURLY_BRACKET
• T_STYLE
• T_SEMICOLON
• T_ASPERAND
• T_COLON
• T_SEMICOLON
• T_COLON
• T_STYLE
  All these constants are defined in vendor/squizlabs/php_codesniffer/src/Util/Tokens.php
  I am not sure in this point, probably, we can include this file to autoload.

5. Constants from file dev/tools/UpgradeScripts/pre_composer_update_2.3.php

• INFO
• WARN
• ERROR
  I suggest skipping this point.

What do you think about this?

[viktym]

@swnsma great job!
I'll review the diff and provide feedback till the end of the week

[viktym]

Hi @swnsma
I agree with your suggestions 1-3

Also, I recommend to ignore constants from 4 using pattern: #Constant T_[A-Z_]+ not found.#

Number 5 you can add in blacklist:

dev/tests/static/testsuite/Magento/Test/Php/_files/phpstan/blacklist/common.txt

Please proceed with implementation of these recommendations and fix the failed Integrity test as well:

Passed: 32740, Failed: 1, Incomplete: 0, Skipped: 0.
Data set: dev/tests/static/framework/tests/unit/testsuite/Magento/PhpStan/Reflection/DataObject/MagicMethodsTest.php
Location of /var/www/html/dev/tests/static/framework/tests/unit/testsuite/Magento/PhpStan/Reflection/DataObject/MagicMethodsTest.php does not match formal namespace: Magento\PhpStan\Reflection\DataObject

[swnsma]

Hi @viktym!
Done!

[viktym]

Hi @swnsma
PR looks good for me
Don't have any objections

[lenaorobei]

@magento run all tests

[swnsma]

@magento run all tests

[lenaorobei]

@magento run all tests

[engcom-Charlie]

@magento run all tests

[engcom-Charlie]

HI @swnsma, could you please look through a failed static tests?

[swnsma]

Fixed.

BTW.
I think this PR, at least, deserves 'Award: test coverage' label, as it introduce tests coverage for the new functionality.

[engcom-Charlie]

@magento run all tests

[swnsma]

@magento run all tests

[m2-assistant]

Hi @swnsma, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.