bueltge / multisite-global-media

Share a media library across the WordPress Multisite network
GNU General Public License v2.0
213 stars 49 forks source link

wpcs and incompatibility with php 7.4 and 8.0 #106

Closed widoz closed 4 years ago

widoz commented 4 years ago

After the introduction of Github Actions I have seen two files Attachments.php and AttachmentsTest.php are not passing the phpcs validation.

The result of the error can be seen in this page https://github.com/bueltge/multisite-global-media/runs/517875774?check_suite_focus=true

Run ./vendor/bin/phpcs
................E. 18 / 18 (100%)

FILE: ...l-media/multisite-global-media/tests/php/unit/AttachmentTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | An error occurred during processing; checking has been
   |       | aborted. The error message was: Trying to access array
   |       | offset on value of type null in
   |       | /home/runner/work/multisite-global-media/multisite-global-media/vendor/wp-coding-standards/wpcs/WordPress/Sniff.php
   |       | on line 1375 (Internal.Exception)
----------------------------------------------------------------------

Time: 1.74 secs; Memory: 12MB

##[error]Process completed with exit code 1.

I was able to fix the issues in Attachments.php (see https://github.com/bueltge/multisite-global-media/commit/504ca11788406f4360328032d7dc3563dcfb634a) even though the result of phpcs does not say anything about the real error which apparently seems to be a global accessing variable.

I was able to replicate the problem locally so it's not something related to Github Action.

But still, I was curious to understand what could be the cause of the problem so I had a look at the file ./vendor/wp-coding-standards/wpcs/WordPress/Sniff.php line 1375 (note, the current version installed by composer of wp-conding-standards/wpcs is the 0.14.1) and I have seen these two lines: https://github.com/WordPress/WordPress-Coding-Standards/blob/0.14.1/WordPress/Sniff.php#L1344 and https://github.com/WordPress/WordPress-Coding-Standards/blob/0.14.1/WordPress/Sniff.php#L1374-L1377

From what I have seen the variable static $last; is initialized after the if conditional at line 1374 (see https://github.com/WordPress/WordPress-Coding-Standards/blob/0.14.1/WordPress/Sniff.php#L1394-L1398) and only if the conditional at line 1374 fails.

The main reason it's because of Notice: Trying to access array offset on value of type null. Try to access to an offseet on a value of tyle null is a notice since php 7.4. Have a look at https://3v4l.org/0XWHY.

Reason is simple, since php 7.4 there's a backward incompatible change

Array-style access of non-arrays ¶ Trying to use values of type null, bool, int, float or resource as an array (such as $null["key"]) will now generate a notice.

See: https://www.php.net/manual/en/migration74.incompatible.php

For that reason and since we cannot update wp-conding-standards/wpcs to a newest version since the 0.14.1 is the latest version before the 1.0.0 and also, it's a dependency of inpsyde/php-coding-standards and I would not update it to avoid conflicts with the inpsyde coding standard package.

My proposal is to exclude tests from being checked against php coding standards until this problem is solved or if we remove a direct access to the global variables.

One thing which I do not understand is why it's related to the global variable access but it's does not happen for other violations of the standard.

bueltge commented 4 years ago

Thanks a lot @widoz Great work for feature php versions and the plugin, good to see your commitment! ❤️