rector icon indicating copy to clipboard operation
rector copied to clipboard

PHP version-dependent constant handling

Open savinmikhail opened this issue 1 year ago • 1 comments

Bug Report

Subject Details
Rector version e.g. v1.1.1
PHP version 8.3.7

I have library, written for php 8.2. I used Rector to create php 7.4 compatible version. In my code I used token contants, like (T_CLASS, T_ENUM) I used the following rector config

return RectorConfig::configure()
    ->withPaths([
        __DIR__ . '/src',
        __DIR__ . '/tests',
    ])
    ->withSets([DowngradeLevelSetList::DOWN_TO_PHP_74]);

After executing, the T_ENUM constant has been still present in my code, which lead to the error on php 7.4 server:

PHP Warning:  Use of undefined constant T_ENUM - assumed 'T_ENUM' (this will throw an Error in a future version of PHP) in /var/www/vendor/savinmikhail/comments-density/src/MissingDocBlockAnalyzer.php on line 40

Minimal PHP Code Causing Issue

 if ($token[0] === T_DOC_COMMENT) {
                $lastDocBlock = $token[1];
            } elseif (in_array($token[0], [T_CLASS, T_TRAIT, T_INTERFACE, T_ENUM, T_FUNCTION], true)) {
                if (empty($lastDocBlock)) {
                    $missingDocBlocks[] = [
                        'type' => 'missingDocblock',
                        'content' => '',
                        'file' => $filename,
                        'line' => $token[2]
                    ];
                }

Expected Behaviour

I think the easiest way would be report of such PHP version-dependent constants

savinmikhail avatar Jun 29 '24 08:06 savinmikhail

constant value set with add if constant exists check seems needed, on downgrade, looking at https://3v4l.org/mHBk2 , the value seems different between PHP 8.1 to PHP 8.3, replace with statement if not onstant exist and php < 8.3, then 336, else 372

samsonasik avatar Jun 29 '24 08:06 samsonasik

@savinmikhail We'll need a minimal reproducer repository to tackle this. Could you share one?

TomasVotruba avatar Jul 02 '24 15:07 TomasVotruba

@TomasVotruba for sure! Here you go https://github.com/savinmikhail/rector_-8704

savinmikhail avatar Jul 02 '24 15:07 savinmikhail

@savinmikhail new rule: DowngradeTEnumConstantRector seems needed, to add:

+ if (! defined('T_ENUM')) {
+      define...
+

Before the statement that use it, there is service ExprInTopStmtMatcher

https://github.com/rectorphp/rector-downgrade-php/blob/c053b9795a81bb9fd5b8d59e05c7556f19267893/src/NodeAnalyzer/ExprInTopStmtMatcher.php#L28

to match Expr for it so we can prepend if check before...

samsonasik avatar Jul 02 '24 15:07 samsonasik

@samsonasik Awesome! Could you handle this?

TomasVotruba avatar Jul 02 '24 15:07 TomasVotruba

I will try

samsonasik avatar Jul 02 '24 16:07 samsonasik

@samsonasik I believe very few people use the T_ENUM constant. Mb the rule should check all PHP constants? See, many of them are version-specific image

savinmikhail avatar Jul 03 '24 01:07 savinmikhail

another way is you can create your own constants.php and register to your composer.json under autoload -> files config, add if php version and defined check there :)

samsonasik avatar Jul 03 '24 06:07 samsonasik

@samsonasik thank you. I dont need this functional, just thought it might be helpful for others

savinmikhail avatar Jul 03 '24 06:07 savinmikhail

Thanks :+1: I guess this can be closed then.

TomasVotruba avatar Jul 03 '24 09:07 TomasVotruba