feat(DowngradePhp73JsonConstRector): throw behavior #133

sylver · 2023-06-20T10:48:02Z

Add a throwing error behavior after any json_encode or json_decode function called with the JSON_THROW_ON_ERROR flag set, since it will be removed.
This is a partial improvement of removing the JSON_THROW_ON_ERROR flag altogether without a proper alternative behavior, but that only works when the flags are directly set in the function call.
If the flags are set from a variable, that would require a much more complex analysis to be 100% accurate, probably beyond Rector actual capabilities.

* add a code sample for `json_decode`

* Try to properly downgrade the throw behavior removed from `json_encode` and `json_decode` with an alternative code snippet added after the function node. * This is a partial solution that only handles when the flags are directly set in the function call.

sylver · 2023-06-20T11:09:13Z

@samsonasik It seems there's a problem in your CI when the PR branch is not from the main repository, so the last check can't pass.

samsonasik · 2023-06-20T11:10:38Z

You can run vendor/bin/rector && composer fix-cs to fix CI

fix CI

sylver · 2023-06-20T12:55:28Z

@samsonasik following your advice, the code is fixed for the CI, but I had also to fix one file outside of the scope of this PR, which was not reported locally.
That's probably unexpected.

samsonasik · 2023-06-20T13:08:35Z

You need to enable zend.assertions=1 to enable assertion error locally. It usually require set startLine and endLine attribute to new node, something like this PR:

Compatibility with BetterReflection 6.x on ClassFromEnumFactory (take 2) rector-src#3207

sylver · 2023-06-20T13:13:06Z

It seems I'm unlucky today, everything was finally fine but some packages got an update and a lot of things are now broken (that are not related to this PR)

  - Upgrading phpstan/phpstan (1.10.19 => 1.10.20): Extracting archive
  - Upgrading rector/rector-src (dev-main ad41e8a => dev-main 3ed219b): Extracting archive
  - Upgrading rector/rector-symfony (dev-main f1020f6 => dev-main 100ca8c): Extracting archive
  - Upgrading rector/rector-phpunit (dev-main 5501d9d => dev-main 8288f71): Extracting archive
  - Upgrading symplify/easy-coding-standard (11.3.4 => 11.4.5): Extracting archive
  - Upgrading tomasvotruba/unused-public (0.1.10 => 0.1.12): Extracting archive

samsonasik · 2023-06-20T13:20:29Z

It seems new phpstan cause error in rector-src as well:


There were 3 errors:

1) Rector\Tests\Php80\Rector\Switch_\ChangeSwitchToMatchRector\ChangeSwitchToMatchRectorTest::test with data set #11
Error: Call to undefined method Rector\Core\PhpParser\Printer\BetterStandardPrinter::pPHPStan_Node_AlwaysRememberedExpr()

I will check if that possibly related.

samsonasik · 2023-06-20T17:51:54Z

I created PR:

Bump PHPStan to 1.10.20 rector-src#4305

for it.

sylver · 2023-06-20T21:45:54Z

Thanks @samsonasik and @TomasVotruba for the fix on rectorphp/rector-src
Tests are running green again locally for this PR, so I hope the CI should be ok too now.

samsonasik · 2023-06-21T03:35:47Z

@TomasVotruba the error:

https://github.com/rectorphp/rector-downgrade-php/actions/runs/5322159313/jobs/9655820170?pr=133#step:5:14

"System error: "Call to undefined method                               
         Rector\Core\PhpParser\Printer\BetterStandardPrinter::pPHPStan_Node_AlwaysRememberedExpr()"

because BetterStandardprinter doesn't have pPHPStan_Node_AlwaysRememberedExpr() method, so extends PHPStan printer may actually needed.

samsonasik · 2023-06-21T04:39:58Z

I create new PR for extends PHPStan printer instead:

[Printer] Extends PHPStan printer on PHPStan 1.10.20 rector-src#4308

samsonasik · 2023-06-21T09:45:20Z

Let see if rectorphp/rector-src#4310 resolve the rector build, let's restasrt build...

samsonasik · 2023-06-21T09:46:50Z

CI seems green now 👍

sylver · 2023-06-21T10:18:40Z

@samsonasik awesome, thanks 🙏

I hope my contribution could be useful and merged if you think so.

TomasVotruba · 2023-06-21T11:28:46Z

Thank you for contribution 👍 lets give this a try

samsonasik · 2023-06-21T11:40:35Z

It seems cause downgrade build error

https://github.com/rectorphp/rector-src/actions/runs/5332525439/jobs/9664202507

I will look into it.

samsonasik · 2023-06-21T11:44:58Z

rules/DowngradePhp73/Rector/ConstFetch/DowngradePhp73JsonConstRector.php

+        $funcCall = match ($stmtExpression->expr::class) {
+            Assign::class => $stmtExpression->expr->expr instanceof FuncCall
+                ? $stmtExpression->expr->expr
+                : null,
+            FuncCall::class => $stmtExpression->expr,
+            default => null,
+        };


downgrade this part seems cause error: https://github.com/rectorphp/rector-src/actions/runs/5332525439/jobs/9664202507#step:15:72

The error seems happen on php 7.4 https://3v4l.org/75qT2#v7.4.33

I think it is because The scope of $stmtExpression->expr is already lost as parent re-printed, the better solution I think is refactor the code for now. I will create new PR.

samsonasik · 2023-06-21T12:20:32Z

I think for inside try-catch, it should use the the catch stmts, see

rectorphp/rector@3236151#diff-3730c571f73f17d036e1d2177280cdb16a21772965d08ad48f23485036b9b03a

should be:

          try {
-                $resource = \json_encode($resource, \JSON_THROW_ON_ERROR);
+                $resource = \json_encode($resource, 0);
                if (\json_last_error() !== \JSON_ERROR_NONE) {
-                    throw new \Exception(\json_last_error_msg());
+                    $resource = \sprintf('resource of type "%s"', \get_debug_type($resource));
                }
            } catch (\JsonException $exception) {
                $resource = \sprintf('resource of type "%s"', \get_debug_type($resource));
            }

samsonasik · 2023-06-21T12:22:28Z

or better, should be something like:

-          try {
-                $resource = \json_encode($resource, \JSON_THROW_ON_ERROR);
+                $resource = \json_encode($resource, 0);
                if (\json_last_error() !== \JSON_ERROR_NONE) {
-                    throw new \Exception(\json_last_error_msg());
+                    $resource = \sprintf('resource of type "%s"', \get_debug_type($resource));
                }
-            } catch (\JsonException $exception) {
-                $resource = \sprintf('resource of type "%s"', \get_debug_type($resource));
-            }

samsonasik · 2023-06-21T12:27:01Z

Hm.., I think inside try catch can just be skipped for add if after Expression to avoid regression, I will create new PR for it.

samsonasik · 2023-06-21T12:34:33Z

#135

samsonasik · 2023-06-21T12:48:26Z

...ctor/ConstFetch/DowngradePhp73JsonConstRector/Fixture/add_throw_on_json_decode_error.php.inc

+        if (json_last_error() !== JSON_ERROR_NONE) {
+            throw new \Exception(json_last_error_msg());
+        }


this is incorrect, as it previously on \Exception to return false, so must be skipped instead to avoid regression, so basically if inside try catch, it must not add the if, here the example:

https://3v4l.org/k3U5V

with new behaviour, it throw exception, which should not.

@samsonasik @TomasVotruba Not sure to follow you here, without the if statement nor the JSON_THROW_ON_ERROR, json_encode() / json_decode() does not raise any error, so the catch is not executed, that's exactly why this PR was made, you're actually removing the precise purpose of it.

The problem resides on the type of the exception thrown to match the one in the catch, you should not remove the if entirely or you defeat the purpose of this PR and we are back to square one.

In PHP < 7.3 that does not throw (initial behavior of DowngradePhp73JsonConstRector) :

https://3v4l.org/1Y2Bv#v7.2.34

try { $data = "\xB1\x31"; // Invalid UTF-8 sequence (binary data) $json = json_encode($data, 0); echo $json; } catch (JsonException $e) { echo 'Error occurred during JSON encoding: ' . $e->getMessage(); }

While this replicate the same behavior as with JSON_THROW_ON_ERROR (goal of my PR) :

https://3v4l.org/lb86Z#v7.2.34

try { $data = "\xB1\x31"; // Invalid UTF-8 sequence (binary data) $json = json_encode($data, 0); if (json_last_error() !== JSON_ERROR_NONE) { throw new \Exception(json_last_error_msg()); } echo $json; } catch (Exception $e) { echo 'Error occurred during JSON encoding: ' . $e->getMessage(); }

Also the exception JsonException does not exist in PHP < 7.3 so your example is not valid in this context, if the catch specify a JsonException, it should be replaced by a standard Exception to be compatible with PHP < 7.3 anyway.

I could have worked that out if you didn't act so fast, with your last merge this PR is basically useless.
It was just needed a little bit of improvement on the exception type (and actually also handle the downgrade of JsonException that I overlooked, thanks for pointing that out)

See https://3v4l.org/k3U5V vs https://3v4l.org/gJkgE

Better approach to use catch stmts instead probably the real solution, but the quick fix is not add if to avoid regression.

sorry, I don't get it, it's like you didn't read what I just said.

The 2 examples you just provided are not valid PHP 7.2.x (since JsonException does not exist for that version)

https://www.php.net/manual/en/class.jsonexception.php

Exception can be used, but need to map for what defined inside catch, or it will fatal error that mean regression.

For rector use case, the downgrade is used to allow installed in php 7.2, to upgrade code from php 5 until php 8.2, so the downgraded code should keep working even opened in php 8.

I thought a "Downgrade" rule could only be applied.. well, for a downgrade. That wouldn't make much sense to apply it the other way around during an upgrade, but again I don't know all the subtlety of Rector.
Would be curious to understand it more, when you'll have the time, I have a few other PRs coming up 😅

I'll go with a custom local rule for now, since with your last changes it does not fix my downgrade problem anymore.

as a side note, this can be rework with more carefull approach, with check if catch Exception or Throwable, or JsonException directly, then map the code below catch inside if, then remove the try catch itself.

See my comment above #133 (comment)

Yeah, I didn't get it at first tbh, but thing is you're trying to avoid the throw/catch entirely, while this is the initial/expected behavior of the code that should be retained, no matter what, else you get a discrepancy between the versions of your code.

I'll try to come up with something else if I have the time, unless you do it first. Thanks for your help and the discussion/debate 😉

No worries, improvement is welcome, just need more carefull approach to avoid regression ;)

I have on list of downgrade improvement, one of them is patch invalid the docblock during downgrade, see rectorphp/rector#8004 but that's not our priority right now as docblock doesn't cause fatal error ;)

No worries, improvement is welcome, just need more carefull approach to avoid regression ;)

btw, how could I have caught locally the regression issue you got before pushing the PR ?
Since all was green both locally and eventually in the CI, I don't know where to look for to reproduce it ?

For this rule itself, for example, we have the following code:

try { $data = "\xB1\x31"; // Invalid UTF-8 sequence (binary data) $json = json_encode($data, JSON_THROW_ON_ERROR); echo $json; } catch (JsonException $e) { echo 'something is wrong'; }

Currently, in php 7.3, it output something is wrong, see https://3v4l.org/GkRKc#v7.3.33

When downgraded to php 7.2, it should keep show something is wrong, see https://3v4l.org/XSJ4W#v7.2.34

This should limit per use-case to avoid general change,eg: Try, then catch JsonException, no finally, no anything else, if that works, we can improve to another improvement :)

sylver added 2 commits June 18, 2023 20:34

chore(DowngradePhp73JsonConstRector): add code sample

3024cff

* add a code sample for `json_decode`

sylver force-pushed the feat/json_throw_error branch from a37bbd1 to 63abfb6 Compare June 20, 2023 10:56

sylver added 2 commits June 20, 2023 13:34

fixup! feat(DowngradePhp73JsonConstRector): throw behavior

82b1833

fix CI

fix(ArrayMergeFromArraySpreadFactory): unused use

603d0e6

TomasVotruba merged commit 9cfcb6a into rectorphp:main Jun 21, 2023
5 checks passed

samsonasik reviewed Jun 21, 2023

View reviewed changes

samsonasik mentioned this pull request Jun 21, 2023

[DowngradePhp73] Refactor DowngradePhp73JsonConstRector so downgrade-able #134

Merged

samsonasik mentioned this pull request Jun 21, 2023

[DowngradePhp73] Do not add if inside try catch for DowngradePhp73JsonConstRector #135

Merged

samsonasik reviewed Jun 21, 2023

View reviewed changes

samsonasik mentioned this pull request Jun 30, 2023

Refactor DowngradeMatchToSwitchRector to ternary, to keep expr === expr design #112

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(DowngradePhp73JsonConstRector): throw behavior #133

feat(DowngradePhp73JsonConstRector): throw behavior #133

sylver commented Jun 20, 2023

sylver commented Jun 20, 2023

samsonasik commented Jun 20, 2023

sylver commented Jun 20, 2023

samsonasik commented Jun 20, 2023

sylver commented Jun 20, 2023 •

edited

Loading

samsonasik commented Jun 20, 2023

samsonasik commented Jun 20, 2023

sylver commented Jun 20, 2023

samsonasik commented Jun 21, 2023

samsonasik commented Jun 21, 2023

samsonasik commented Jun 21, 2023

samsonasik commented Jun 21, 2023

sylver commented Jun 21, 2023

TomasVotruba commented Jun 21, 2023

samsonasik commented Jun 21, 2023

samsonasik Jun 21, 2023

samsonasik Jun 21, 2023

samsonasik Jun 21, 2023

samsonasik commented Jun 21, 2023

samsonasik commented Jun 21, 2023

samsonasik commented Jun 21, 2023

samsonasik commented Jun 21, 2023

samsonasik Jun 21, 2023

sylver Jun 21, 2023 •

edited

Loading

samsonasik Jun 21, 2023 •

edited

Loading

sylver Jun 21, 2023 •

edited

Loading

samsonasik Jun 21, 2023

sylver Jun 21, 2023

sylver Jun 21, 2023

samsonasik Jun 21, 2023

sylver Jun 21, 2023

samsonasik Jun 21, 2023

feat(DowngradePhp73JsonConstRector): throw behavior #133

feat(DowngradePhp73JsonConstRector): throw behavior #133

Conversation

sylver commented Jun 20, 2023

sylver commented Jun 20, 2023

samsonasik commented Jun 20, 2023

sylver commented Jun 20, 2023

samsonasik commented Jun 20, 2023

sylver commented Jun 20, 2023 • edited Loading

samsonasik commented Jun 20, 2023

samsonasik commented Jun 20, 2023

sylver commented Jun 20, 2023

samsonasik commented Jun 21, 2023

samsonasik commented Jun 21, 2023

samsonasik commented Jun 21, 2023

samsonasik commented Jun 21, 2023

sylver commented Jun 21, 2023

TomasVotruba commented Jun 21, 2023

samsonasik commented Jun 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samsonasik commented Jun 21, 2023

samsonasik commented Jun 21, 2023

samsonasik commented Jun 21, 2023

samsonasik commented Jun 21, 2023

Choose a reason for hiding this comment

sylver Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

samsonasik Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

sylver Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sylver commented Jun 20, 2023 •

edited

Loading

sylver Jun 21, 2023 •

edited

Loading

samsonasik Jun 21, 2023 •

edited

Loading

sylver Jun 21, 2023 •

edited

Loading