[Php85] Remove calls to deprecated no-op functions

[mttsch]

No description provided.

[samsonasik]

On php <8.4, is that function necessary? If needed, then downgrade rule need to exists first, or make a rule to change code like:

- finfo_close($info);
+if (PHP_VERSION_ID < 80500) {
+    finfo_close($info);
+}

[mttsch]

@samsonasik According to the RFC, all of them have been no-ops at least since PHP 8.1, see listing at https://wiki.php.net/rfc/deprecations_php_8_5#deprecate_no-op_functions_from_the_resource_to_object_conversion

PS: I will fix merge conflicts tomorrow as all of the PHP 8.5 PRs change php85.php.

[samsonasik]

I prefer to have code that less bc break even downgraded to php 7 on our downgrade rule:

https://github.com/rectorphp/rector-downgrade-php

so if there is no downgrade rule, imo, the code should have backward compatible code.

[mttsch]

I am not familiar with the downgrade package but wouldn't most people applying this rule or the PHP 8.5 rule set want to stay on PHP 8.5+, so that such an additional condition would be useless in this case and would just add more code instead of removing the "useless" code.

Can you point me to existing examples in the Rector source code for such a scenario that I came have a look at? 🙂

[samsonasik]

rector-src itself shipped as php 7.4 compatible code, include the prefixed "vendor"

https://github.com/rectorphp/rector/tree/main/vendor

which if the code exists, and rector for example require php 8.5, and the code removed, this should have flag handling to cover that, except, the list of function doesn't have any functionality in previous versions.

If there is no way to do that, possibly can be an optional list collection of rules.

[staabm]

Agree we should rewrite it to a PHP_VERSION based check.

A separate rector rule can remove if (PHP_VERSION < 1234) based code as dead code according to composer.json min version

[mttsch]

Can you provide me with an existing code example where an expression (FuncCall) is replaced with a statement to see how to properly do that?

[staabm]

e.g. InlineIfToExplicitIfRector or TernaryFalseExpressionToIfRector turns a expression into a IF statement

[mttsch]

I opted for a reusable rector rule that wraps certain function calls in a PHP_VERSION_ID condition.

To keep it simple for now, it only considers expressions with only the function call, not in assignments and conditions, for example.

But then, three functions, finfo_close(), imagedestroy(), and xml_parser_free() have a return type.

Do you think that these cases should also be considered with a return type expectation of true?

[staabm]

a similar rector (separate PR) would be usefull for MethodCall, to cover e.g. ReflectionProperty::setAccessible()

see phpstan/phpstan-src@466a3de

[mttsch]

@staabm Actually, I wanted to do that but then thought that the deprecation of these methods where voted against but I confused that with the ReflectionParameter::allowsNull() vote.

I will have look at that later once this PR is through.

[samsonasik]

It seems if this rule is combined with RemovePhpVersionIdCheckRector, the change will always flip flop never ending:

withRules([
     RemovePhpVersionIdCheckRector::class,
])
->withConfiguredRule(WrapFuncCallWithPhpVersionIdCheckerRector::classs, [
            new WrapFuncCallWithPhpVersionIdChecker('curl_close', 80500),
]);

so probably something to consider is check what existig rules registered to avoid repetive flip flop on repetitive run.

[mttsch]

@samsonasik Done.

Can you please mark the appropriate discussions are solved to avoid to many multiple "open" discusssions?

(I will rebase again due to merge conflicts.)

[samsonasik]

yes, marked as resolved 👍

[samsonasik]

Looks good to me 👍

[samsonasik]

Thank you @mttsch

[TomasVotruba]

I'm going through some failing tests in unrelated feature, and just got to this rule.
Looking at result, it creates BC breaks on PHP 8.5 as it changes code with no replacement.

Correct me if I'm wrong.

Rector should focus on deprecation that have a replacement, not just wrap everything deprecated with ifs. That does not bring value and silently creates BC breaks instead. PHPStan is with https://github.com/phpstan/phpstan-deprecation-rules is here for that and require human touch.

[samsonasik]

@TomasVotruba the issue without wrapping is, when code downgraded, there is no replacement for removed code, so to keep both upgrade and downgrade work, the wrap if is needed.

For example:

curl_exec();
curl_close();

On PHP 8.5, curl_close() is deprecated, if it removed, downgrading is impossible if it already removed as curl_close() is needed in PHP 8.4 and below.

[samsonasik]

and the deprecation notice cause unwanted behaviour for user that see the notice in the application, and may be removed in future php 8.6 or php 9, so to keep works on both upgrade and downgrade, the wrap is needed.

[TomasVotruba]

@samsonasik I see. In that case we should wait till these functions are actually removed. I'd be surprised to upgrade to PHP 8.5 and suddently have more lines in my project around deprecated functions.

Are there any such functions in Rector downgraded codebase?

[samsonasik]

Yes, we use the wrap on purpose, for example, the setAccessible(true) downgrade use case

https://github.com/rectorphp/rector-downgrade-php/blob/2c4aa1869405cb6f7f4336a4a5c8f99a8a96828b/rules-tests/DowngradePhp81/Rector/StmtsAwareInterface/DowngradeSetAccessibleReflectionPropertyRector/Fixture/some_class.php.inc#L26-L28

without the wrap, when code read by php 8.5 that setAccessible(true) is deprecated, it will show notice, with the wrap, it works ok.

The purpose is, when code downgraded, it works, read by greater Php version, it keep works, the use case is on rector code itself when run php 8.5

[TomasVotruba]

In downgrade it makes sense, but upgrade path should be independent and not clutter codebase. Think of them as 2 processes that have no idea about each other.

Code I write in PHP 8.5 should work on downgrade, regardless if it was upgraded with Rector or not.

[TomasVotruba]

Are there any such functions in Rector downgraded codebase?

[samsonasik]

With file search, they are exists

while possibly only on require-dev, they are exists.

[TomasVotruba]

Yes, these are dev, one of them even should not be part of the released package. We don't do any curl_ calls.

[TomasVotruba]

I'll replace it with func call removal.

[TomasVotruba]

Ref #7520