PHP 7.2 Magic autoload deprecation by wimg · Pull Request #540 · PHPCompatibility/PHPCompatibility

wimg · 2017-12-01T17:42:05Z

Refs :

Fix issue with DeprecatedMagicAutoloadSniff

jrfnl

Looks good. I'm looking forward to merging this as the last PHP 7.2 deprecation 🎉!

I'm wondering if a little more protection against false positives is warranted.
As far as I know, the __autoload function will only be recognized as an autoloader when defined in the global namespace, i.e. not in a namespaced file or when a method in a class is called __autoload.
Though I'm starting to doubt myself after seeing your test case.

The sniff as it is now, would in those cases also throw warnings.
If a change is needed for this, you can probably use these existing utility functions to execute the needed checks:

validDirectScope() - to determine if the function $stackPtr is defined in a T_CLASS, T_ANON_CLASS, T_INTERFACE or T_TRAIT.
determineNamespace() - to determine whether the function $stackPtr is defined in a namespaced file.

If this is changed, the unit tests would also need to reflect this.

jrfnl · 2017-12-01T18:27:35Z

phpcs.xml.dist

 		<!-- Having a @see or @internal tag before the @param tags is fine. -->
 		<exclude name="Generic.Commenting.DocComment.ParamNotFirst"/>
+		<!-- We prefer seeing nicely looking docblock comments -->
+		<exclude name="Generic.Commenting.DocComment.TagValueIndent"/>


This should be a separate PR.

What are you trying to solve here ? Could you give a code example ?

Will make a separate PR of it.
See https://travis-ci.org/wimg/PHPCompatibility/jobs/310161506

jrfnl · 2017-12-01T18:28:37Z

PHPCompatibility/Sniffs/PHP/DeprecatedMagicAutoloadSniff.php

+            return;
+        }
+
+        $this->addMessage($phpcsFile, 'Use of __autoload() function is deprecated since PHP 7.2', $stackPtr, false);


As there is no error/warning toggle needed at this moment, why not call the $phpcsFile->addWarning() method directly ?

100% true :-)

jrfnl · 2017-12-01T18:30:04Z

PHPCompatibility/Tests/sniff-examples/deprecated_magic_autoload.php

+}
+
+class foo {
+    function __autoload($someclass) {


Are you sure this code works as an autoloader ? I always though the __autoload() function had to be defined in the global namespace ?

jrfnl · 2017-12-01T18:49:16Z

Just re-read the RFC: looks like my initial remark about it only applying to global functions was correct:

Proposed action: A deprecation notice is thrown when a global function with name __autoload() is encountered during compilation.

Maybe we should file a bug regarding the migration guide ? It is called a method there, while it should be called function.

wimg · 2017-12-02T02:10:13Z

So the question is : what do we do about declarations of it outside the global namespace ?
The new code will generate a warning, regardless of the location...

jrfnl · 2017-12-02T02:42:24Z

So the question is : what do we do about declarations of it outside the global namespace ?
The new code will generate a warning, regardless of the location...

I'm not sure I understand what you are asking.
Just looking at the code, I would expect it to now only generate a warning when the function declaration is found in the global namespace and to no longer throw warnings when it's either namespaced or in a valid direct scope....

For that matter: I'm missing a test for a namespaced function declaration, both scoped and non-scoped namespace.

Let me check out the branch and test it, maybe that'll help me understand what you mean.

jrfnl · 2017-12-02T02:56:26Z

PHPCompatibility/Sniffs/PHP/DeprecatedMagicAutoloadSniff.php

+            return;
+        }
+
+        if ($this->validDirectScope($phpcsFile, $stackPtr, array('T_CLASS', 'T_ANON_CLASS', 'T_INTERFACE', 'T_TRAIT')) === true) {


Ah! Found it. The ValidDirectScope() method expects the string tokens as keys in the array, so it can use the more efficient isset().

So it expects the array to be passed like this:

$excludeScopes = array( 'T_CLASS' => true, 'T_ANON_CLASS' => true, 'T_INTERFACE' => true, 'T_TRAIT' => true, 'T_NAMESPACE' => true, // = Only scoped namespaces, non-scoped still needs to be checked in another way. ); if ($this->validDirectScope($phpcsFile, $stackPtr, $excludedScopes) === true) { ....

As $excludedScopes does not change between calls to the sniff, it should probably be declared as a private property of the sniff.

Also, the validDirectScope() method is quicker and simpler than the determineNamespace() method, so the order of the checks should probably be reversed for efficiency of the sniff.

jrfnl

Few more small remarks.

Regarding the build failure: this is to do with PHP 5.3 in combination with PHPCS < 2.4.0.

Traits aren't recognized as a separate token yet.
Interfaces are, but the tokens within the interface don't get the scoped condition correctly.

Considering it's such an old PHP and PHPCS version, I'd be happy to just bypass those unit tests for that particular combination.

jrfnl · 2017-12-02T16:20:34Z

PHPCompatibility/Tests/Sniffs/PHP/DeprecatedMagicAutoloadSniffTest.php

+            array(self::TEST_FILE, 14),
+            array(self::TEST_FILE, 18),
+            array(self::TEST_FILE, 24),
+            array(self::TEST_FILE_NAMESPACED, 6),


These are all one line off -> 5, 10, 15, 20, 26

jrfnl · 2017-12-02T16:21:07Z

PHPCompatibility/Tests/Sniffs/PHP/DeprecatedMagicAutoloadSniffTest.php

+     *
+     * @return void
+     */
+    public function testIsNotAffected($testFile, $line)


Maybe call it testNoFalsePositives() for consistency with all the other test files ?

jrfnl · 2017-12-02T16:23:18Z

PHPCompatibility/Tests/Sniffs/PHP/DeprecatedMagicAutoloadSniffTest.php

+     */
+    public function testIsDeprecated($line)
+    {
+        $file = $this->sniffFile(self::TEST_FILE, '7.1');


You could remove this first test in favour of an overal testNoViolationsInFileOnValidVersion() test for 7.1 checking the complete file (see other unit test files)

Good point. Fixed in next commit.

jrfnl · 2017-12-02T16:46:51Z

PHPCompatibility/Sniffs/PHP/DeprecatedMagicAutoloadSniff.php

+class DeprecatedMagicAutoloadSniff extends Sniff
+{
+    /**
+     * Scopes to exclude when testing using validDirectScope


This documentation is incorrect. The ValidDirectScope() method needs to actually look for these scopes. In the sniff code itself, no warning is then thrown if any of them are found.

Can you elaborate on this ?

When I read this comment, I'd think that those scopes are excluded when calling validDirectScope(), while in actual fact, you are looking for those scopes.

Ah ok thanks ;-)

jrfnl · 2017-12-02T16:49:06Z

PHPCompatibility/Sniffs/PHP/DeprecatedMagicAutoloadSniff.php

+        'T_TRAIT'      => true,
+        'T_NAMESPACE'  => true, // = Only scoped namespaces, non-scoped still needs to be checked in another way.
+    );
+


You seem to have trailing whitespace here which you may want to remove. Strange that the PHPCS run is not complaining about that.... hmm.... will need to look into that.

jrfnl · 2017-12-02T17:16:37Z

PHPCompatibility/Tests/sniff-examples/deprecated_magic_autoload.php

+    function __autoload($someclass) {
+        echo 'I am the autoloader in an anonymous class (which makes no sense) - I am deprecated from PHP 7.2 onwards';
+    }
+});


Nitpick: there should be a new line at the end of the file

jrfnl · 2017-12-02T17:16:45Z

PHPCompatibility/Tests/sniff-examples/deprecated_magic_autoload_namespaced.php

+    function __autoload($someclass) {
+        echo 'I am the autoloader in an anonymous class (which makes no sense) - I am deprecated from PHP 7.2 onwards';
+    }
+});


Nitpick: there should be a new line at the end of the file

jrfnl · 2017-12-06T01:41:44Z

PHPCompatibility/Tests/Sniffs/PHP/DeprecatedMagicAutoloadSniffTest.php

+    public function testIsNotAffected($testFile, $line, $isTrait)
+    {
+        if ($isTrait === true && self::$recognizesTraitsOrInterfaces === false) {
+            $this->markTestSkipped('Traits are not recognized on PHPCS < 2.4.0 in combination with PHP < 5.4');


Not a merge blocker, but just documenting this: the skip message is not entirely accurate as it also applies to interfaces where scope is not being recognized (yet).

jrfnl · 2017-12-06T01:43:10Z

PHPCompatibility/Tests/Sniffs/PHP/DeprecatedMagicAutoloadSniffTest.php

+            array(self::TEST_FILE, 14, true),
+            array(self::TEST_FILE, 18, true),
+            array(self::TEST_FILE, 24, false),
+            array(self::TEST_FILE_NAMESPACED, 6, false),


Shouldn't this be line 5 ?

jrfnl · 2017-12-06T01:43:21Z

PHPCompatibility/Tests/Sniffs/PHP/DeprecatedMagicAutoloadSniffTest.php

+            array(self::TEST_FILE, 18, true),
+            array(self::TEST_FILE, 24, false),
+            array(self::TEST_FILE_NAMESPACED, 6, false),
+            array(self::TEST_FILE_NAMESPACED, 11, false),


Shouldn't this be line 10 ?

wimg and others added 7 commits November 19, 2016 14:04

PHP 7.2 deprecations - part 1

91fc694

Debug for jrf

e162fce

Fix issue with DeprecatedMagicAutoloadSniff

759a4e9

Merge pull request #296 from jrfnl/wimg/php72-deprecations

c8076fa

Fix issue with DeprecatedMagicAutoloadSniff

Merge remote-tracking branch 'origin/php72-deprecations'

d67208c

Added: Deprecated __autoload in PHP 7.2

6419e3e

Removed: old implementation style

00c2ad8

wimg added this to the 8.1.0 milestone Dec 1, 2017

wimg requested a review from jrfnl December 1, 2017 17:42

wimg added 2 commits December 1, 2017 18:50

Lint check fixed

eab7e87

Fixing docblock comment sniff failure on Travis run

d4381be

jrfnl reviewed Dec 1, 2017

View reviewed changes

wimg added 2 commits December 2, 2017 03:10

Addressed comments, removed CS change

33cf41b

validDirectScope array should contain string type tokens

de961ce

jrfnl reviewed Dec 2, 2017

View reviewed changes

wimg added 4 commits December 2, 2017 14:36

Fixed scope tests and adding namespaced test

07421a7

Adding namespace test file + Removed unused code

3795c56

Fixed code style

941fccf

Fixed coding style part 2

06441ce

jrfnl reviewed Dec 2, 2017

View reviewed changes

wimg added 3 commits December 6, 2017 01:30

Traits can not be checked in PHPCS < 2.4.0 and PHP < 5.4

5a4c00c

Interface exception in test + codestyle change

06ef8d2

Made variable for scope check more clear

df3b703

jrfnl reviewed Dec 6, 2017

View reviewed changes

Fixed test lines

015fb43

jrfnl approved these changes Dec 6, 2017

View reviewed changes

jrfnl merged commit 04651f6 into master Dec 6, 2017

jrfnl deleted the magic-autoload-deprecations branch December 6, 2017 10:02

ghost mentioned this pull request Dec 28, 2017

Update wimg/php-compatibility in / from 8.0.1 to 8.1.0 vfalies/php7compatibility#2

Merged

jrfnl added the Type: enhancement label Mar 8, 2018

Uh oh!

Conversation

wimg commented Dec 1, 2017

Uh oh!

jrfnl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrfnl commented Dec 1, 2017

Uh oh!

wimg commented Dec 2, 2017

Uh oh!

jrfnl commented Dec 2, 2017

Uh oh!

jrfnl Dec 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrfnl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jrfnl left a comment •

edited

Loading

jrfnl Dec 2, 2017 •

edited

Loading

jrfnl left a comment •

edited

Loading