Incorrect detection of PHP 4-style constructors by Universal.CodeAnalysis.ConstructorDestructorReturn

[anomiex]

Bug Description

If a __construct() method exists (and you're not still on PHP 4), that is the constructor and any method named like a PHP 4-style constructor is not a constructor. The Universal.CodeAnalysis.ConstructorDestructorReturn sniff treats it as a PHP 4-style constructor anyway.

Also, unless you're using PHP between 5.3.0 and 5.3.2, namespaced classes cannot have PHP 4-style constructors. The Universal.CodeAnalysis.ConstructorDestructorReturn sniff treats it as a PHP 4-style constructor anyway.

Also of note is that PHP 4-style constructors were removed in PHP 8. As this package does not seem to have a "minimum PHP version" setting, I'd recommend using a different code for PHP 4-style constructors so a project that doesn't support older versions of PHP can exclude "Universal.CodeAnalysis.ConstructorDestructorReturn.ReturnValueFoundPHP4" (or whatever name you choose) in its configuration.

Given the following reproduction Scenario

The issue happens when running this command:

vendor/bin/phpcs -s --standard=Universal --sniffs=Universal.CodeAnalysis.ConstructorDestructorReturn test.php test2.php

... over a file containing this code:

<?php

class Foo {
    public function __construct() {
        echo __METHOD__ . "\n";
    }

    public function foo() {
        echo __METHOD__ . "\n";
        return true;
    }
}

... and over a second file containing this code:

<?php

namespace NS;

class Foo {
    public function foo() {
        echo __METHOD__ . "\n";
        return true;
    }
}

I'd expect the following behaviour

No errors or warnings

Instead this happened

FILE: /tmp/test.php
--------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------------------------------------------------------------------------------------
 10 | WARNING | A PHP 4-style constructor can not return a value. Found: "return true;"
    |         | (Universal.CodeAnalysis.ConstructorDestructorReturn.ReturnValueFound)
--------------------------------------------------------------------------------------------------------------------------------------------------------------


FILE: /tmp/test2.php
-------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------------------------------------------------------
 8 | WARNING | A PHP 4-style constructor can not return a value. Found: "return true;"
   |         | (Universal.CodeAnalysis.ConstructorDestructorReturn.ReturnValueFound)
-------------------------------------------------------------------------------------------------------------------------------------------------------------

Environment

Environment	Answer
PHP version	8.0.26
PHP_CodeSniffer version	3.7.1
PHPCSExtra version	dev-develop 0f55c12
PHPCSUtils version	1.0.0
Install type	Composer project local

Additional Context (optional)

See https://3v4l.org/3CAYK and https://3v4l.org/dpRB6 for verifications of the described behaviors. Or just require the test files and construct instances of the classes.

Tested Against develop branch?

• I have verified the issue still exists in the develop branch of PHPCSExtra.

[jrfnl]

Hiya @anomiex, thanks again for this report. Very happy to see you stress-testing the sniffs ;-)

Looks like you are reporting multiple issues here, so I will respond to each part of the report in turn.

Also, unless you're using PHP between 5.3.0 and 5.3.2, namespaced classes cannot have PHP 4-style constructors. The Universal.CodeAnalysis.ConstructorDestructorReturn sniff treats it as a PHP 4-style constructor anyway.

This is a known issue and slated to be fixed in a future release in combination with a namespace tracker becoming available in PHPCSUtils (also see the PHPCSUtils roadmap).

I could add a temporary solution for now, but I previously decided against this, as such a temporary solution would have a significant (negative) impact on the performance of the sniff, i.e. the sniff would likely become ~4x slower.

Is it acceptable to you to leave this for now until a future release ?

If a __construct() method exists (and you're not still on PHP 4), that is the constructor and any method named like a PHP 4-style constructor is not a constructor. The Universal.CodeAnalysis.ConstructorDestructorReturn sniff treats it as a PHP 4-style constructor anyway.

If I remember correctly, there is more nuance to it than that.

If a __construct() method exists (and you're not still on PHP 4) as well as a PHP4-style constructor, both are regarded as constructors by PHP (prior to PHP 8.0).

• The __construct() method, however, has precedence, so will be favoured over the PHP4-style constructor.
• And PHP will not throw a deprecation notice if both exist.

So, in that regards, I believe the sniff is behaving correctly.

Also of note is that PHP 4-style constructors were removed in PHP 8. As this package does not seem to have a "minimum PHP version" setting, I'd recommend using a different code for PHP 4-style constructors so a project that doesn't support older versions of PHP can exclude "Universal.CodeAnalysis.ConstructorDestructorReturn.ReturnValueFoundPHP4" (or whatever name you choose) in its configuration.

While it is true that this package doesn't have a "minimum PHP version" setting, PHPCS actually has a native php_version configuration option, which this sniff could respect.
Note: this config option should, IMO, normally be set using --runtime-set php_version 80100, so it only applies to the current PHPCS run, not to all PHPCS runs for a certain install.
Note: the config option can also be added to a custom ruleset, like so: <config name="php_version" value="80024"/>.

Something similar is already done in the Universal.Arrays.DuplicateArrayKey sniff.

This would basically come down to the sniff not throwing errors for PHP4-style constructors if the php_version would be set to '80000' or higher.

How does that sound to you as a solution ?

Note: this php_version configuration option is not used by the PHPCompatibility standard (which I have a feeling you are familiar with) as PHPCompatibility needs to know the range of supported PHP versions to provide correct results,

[jrfnl]

Note: I've added both the bug and the enhancement labels.

I consider the first issue - "namespaced classes cannot have PHP 4-style constructors" - a (known) bug.

The second issue about (not) reporting on PHP4-style constructors in combination with PHP 8, I consider a potential enhancement as it would add a new feature to the sniff (respecting the php_version).

[anomiex]

Is it acceptable to you to leave this for now until a future release ?

Sure. It's easy to work around with a // phpcs:ignore Universal.CodeAnalysis.ConstructorDestructorReturn -- false positive in the relevant place, and we only have one instance of this in our code.

If a __construct() method exists (and you're not still on PHP 4) as well as a PHP4-style constructor, both are regarded as constructors by PHP (prior to PHP 8.0).

I'm not clear on what you mean by "both are regarded as constructors". The PHP4-style function isn't called for new when __construct() exists. What else is there that makes it be regarded as a constructor?

While it is true that this package doesn't have a "minimum PHP version" setting, PHPCS actually has a native php_version configuration option, which this sniff could respect.

Cool, I didn't know that. That sounds like it could be a workable option. 👍

Note: this php_version configuration option is not used by the PHPCompatibility standard (which I have a feeling you are familiar with) as PHPCompatibility needs to know the range of supported PHP versions to provide correct results,

Yeah, we use that one too. 🙂

[jrfnl]

Is it acceptable to you to leave this for now until a future release ?

Sure. It's easy to work around with a // phpcs:ignore Universal.CodeAnalysis.ConstructorDestructorReturn -- false positive in the relevant place, and we only have one instance of this in our code.

Once the other things here are finished, I think it may be good to move that part to a separate issue.

If a __construct() method exists (and you're not still on PHP 4) as well as a PHP4-style constructor, both are regarded as constructors by PHP (prior to PHP 8.0).

I'm not clear on what you mean by "both are regarded as constructors". The PHP4-style function isn't called for new when __construct() exists. What else is there that makes it be regarded as a constructor?

See the wording in the RFC: https://wiki.php.net/rfc/remove_php4_constructors

In practice, PHP 4 constructors, when used in conjunction with __construct() methods, are normally set up to call the __construct() method and mirror their behaviour so the code is compatible with both PHP 4 as well as PHP 5, like so:

class Foo {
    public function __construct() {
        // Do something.
    }

    public function Foo {
        self::__construct();
    }
}

Now, if the constructor returns something, this will generally be mirrored by the PHP4-style constructor, in which case, both should be flagged IMO.

class Foo {
    public function __construct() {
        return $this;
    }

    public function Foo {
        return self::__construct();
    }
}

While it is true that this package doesn't have a "minimum PHP version" setting, PHPCS actually has a native php_version configuration option, which this sniff could respect.

Cool, I didn't know that. That sounds like it could be a workable option. 👍

Want to give PR #208 a whirl to see if that works for you ?

[jrfnl]

@anomiex I've merged the PR now. Would be nice if you could try it out still, but I intend to release the fix some time over the next two days.

[anomiex]

Now, if the constructor returns something, this will generally be mirrored by the PHP4-style constructor, in which case, both should be flagged IMO.

Personally I can't say I'd worry about that case unless the intent is to still check PHP 4 code. But I won't push on it.

Want to give PR #208 a whirl to see if that works for you ?

Confirmed, it works in testing.

[jrfnl]

Thanks for testing @anomiex ! I've released the fix in the 1.0.2 version.

[jrfnl]

@anomiex As it is taking longer than anticipated before the "better" solution for the PHP4-style methods in namespaced classes is available, I'm putting a fix in now anyway. See PR #272. The fix will be included in the next release.

[jrfnl]

The fix has now been released: https://github.com/PHPCSStandards/PHPCSExtra/releases/tag/1.1.2