Running a PHP code sniff against fresh install for PHP compatibility with PHP 7 shouldn't find errors.

[waynetheisinger]

Description (*)

In migrating to OpenMage I ran a PHP code sniff for PHP7 compatibility - I was expecting to need to patch third-party modules. However, because PHP7 is the minimum requirement I wasn't expecting core libraries to have errors.

There were hundreds of Mcrypt warnings but I notice you are using lib/mcryptcompat/mcrypt.php to polyfill mcrypt so this is a false positive.

The link below contains other errors. They are mostly coming from PEAR and Zend. It is possible/probable that the Mage code isn't calling the methods with the problems.

php7sniffs_core.txt

Feature request

1. If the core code isn't calling the methods with errors, then it would be good to provide a set of custom rules so that PHP code sniff ignores those methods.
2. If the core code is calling the method, then we should either fork the library code, ie the project takes over development of the library for ongoing fixes, or patch the code using something like https://github.com/cweagans/composer-patches

Expected behavior (*)

Running a PHP code sniff against fresh install for PHP compatibility with PHP 7 shouldn't find errors.

Benefits

PHP code sniff can be part of a CI/CD pipeline Future stability will be more reliable when adding third-party modules

[waynetheisinger]

Actually even if the core isn't using the method it is conceivable that a third-party module might, therefore would fixing the problems be preferable to writing a custom rule to ignore them??

[waynetheisinger]

Probably a rule could be written that ignores the known errors in the libraries' functions but errors if said function is called from elsewhere in the codebase.

Obviously this rule will need to also ban other methods in the core libraries that call the troublesome library functions.

[colinmollenhour]

I'd really like to remove all code that doesn't currently serve a valid purpose. E.g. #903 #952 #374 We just need to come to some consensus as to how much to remove, do the review, testing, etc.. So if we did that then it would be a good idea to fix code sniffer errors, but I don't think anyone wants to fix errors in those libs if we aren't even using them. Also see #947

[waynetheisinger]

I agree - so we fork the libraries and then remove all code that isn't being used. I've started this by using PHPstorm to identify dead code... I'll post back here the results of my research.

~~though thinking about this not sure how reliable the inspection tool will be, I'm guessing it won't understand the autoloader pattern... anyway when its finished running I'll at least include what it's found, though, as I say, thinking about it, I've lost faith in its usefulness.~~

The report produces mostly false positives because it doesn't understand the xml based autoloader.

[Flyingmana]

by now we got a lot further with php compatibility, and use various tools to check the code statically. Also we have a lot of users with current php versions.

Therefore I close this issue for now, feel free to open a new issue, if you find further issues, (or reopen here, if you want us to also add this specific codesniff)

[sreichel]

This is issue is still valid.

We have a PHP compatibility check, but it does not fail on error.

Just check current output from github action workflow.

[sreichel]

Cant re-open ...

https://github.com/OpenMage/magento-lts/actions/runs/6429470742/job/17458623872?pr=3582