Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(DowngradePhp73JsonConstRector): throw behavior #133

Merged
merged 4 commits into from
Jun 21, 2023
Merged

feat(DowngradePhp73JsonConstRector): throw behavior #133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3024cff
chore(DowngradePhp73JsonConstRector): add code sample
sylver Jun 18, 2023
63abfb6
feat(DowngradePhp73JsonConstRector): throw behavior
sylver Jun 19, 2023
82b1833
fixup! feat(DowngradePhp73JsonConstRector): throw behavior
sylver Jun 20, 2023
603d0e6
fix(ArrayMergeFromArraySpreadFactory): unused `use`
sylver Jun 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 99 additions & 0 deletions ...r/ConstFetch/DowngradePhp73JsonConstRector/Fixture/add_throw_on_json_decode_error.php.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
<?php

namespace Rector\Tests\DowngradePhp73\Rector\ConstFetch\DowngradePhp73JsonConstRector\Fixture;

// Class
class DowngradePhp73JsonConstRectorFixture
{
public function method_json_decode(string $json): string | bool
{
try {
$payload = json_decode($json, null, 512, JSON_THROW_ON_ERROR);
return $payload;
} catch (\Exception $e) {
return false;
}
}

public function method_json_decode_multi_flags(string $json): string | bool
{
try {
$payload = json_decode($json, null, 512, JSON_OBJECT_AS_ARRAY | JSON_THROW_ON_ERROR);
return $payload;
} catch (\Exception $e) {
return false;
}
}
}

// Function
function func_json_decode(string $json): string | bool
{
try {
$payload = json_decode($json, null, 512, JSON_THROW_ON_ERROR);
return $payload;
} catch (\Exception $e) {
return false;
}
}

// Standalone
$decode = json_decode($json, null, 512, JSON_THROW_ON_ERROR);

?>
-----
<?php

namespace Rector\Tests\DowngradePhp73\Rector\ConstFetch\DowngradePhp73JsonConstRector\Fixture;

// Class
class DowngradePhp73JsonConstRectorFixture
{
public function method_json_decode(string $json): string | bool
{
try {
$payload = json_decode($json, null, 512, 0);
if (json_last_error() !== JSON_ERROR_NONE) {
throw new \Exception(json_last_error_msg());
}
return $payload;
} catch (\Exception $e) {
return false;
}
}

public function method_json_decode_multi_flags(string $json): string | bool
{
try {
$payload = json_decode($json, null, 512, JSON_OBJECT_AS_ARRAY);
if (json_last_error() !== JSON_ERROR_NONE) {
throw new \Exception(json_last_error_msg());
}
return $payload;
} catch (\Exception $e) {
return false;
}
}
}

// Function
function func_json_decode(string $json): string | bool
{
try {
$payload = json_decode($json, null, 512, 0);
if (json_last_error() !== JSON_ERROR_NONE) {
throw new \Exception(json_last_error_msg());
}
Comment on lines +84 to +86
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@sylver sylver Jun 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)

Copy link
Member

@samsonasik samsonasik Jun 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@sylver sylver Jun 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😉

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

return $payload;
} catch (\Exception $e) {
return false;
}
}

// Standalone
$decode = json_decode($json, null, 512, 0);
if (json_last_error() !== JSON_ERROR_NONE) {
throw new \Exception(json_last_error_msg());
}

?>
93 changes: 93 additions & 0 deletions ...r/ConstFetch/DowngradePhp73JsonConstRector/Fixture/add_throw_on_json_encode_error.php.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
<?php

namespace Rector\Tests\DowngradePhp73\Rector\ConstFetch\DowngradePhp73JsonConstRector\Fixture;

// Class
class DowngradePhp73JsonConstRectorFixture
{
public function method_json_encode(string $json): string | bool
{
try {
json_encode($json, JSON_THROW_ON_ERROR);
} catch (\Exception $e) {
return false;
}
}

public function method_json_encode_multi_flags(string $json): string | bool
{
try {
json_encode($json, JSON_THROW_ON_ERROR | JSON_PRETTY_PRINT);
} catch (\Exception $e) {
return false;
}
}
}

// Function
function func_json_encode(string $json): string | bool
{
try {
json_encode($json, JSON_THROW_ON_ERROR);
} catch (\Exception $e) {
return false;
}
}

// Standalone
json_encode($json, JSON_THROW_ON_ERROR);

?>
-----
<?php

namespace Rector\Tests\DowngradePhp73\Rector\ConstFetch\DowngradePhp73JsonConstRector\Fixture;

// Class
class DowngradePhp73JsonConstRectorFixture
{
public function method_json_encode(string $json): string | bool
{
try {
json_encode($json, 0);
if (json_last_error() !== JSON_ERROR_NONE) {
throw new \Exception(json_last_error_msg());
}
} catch (\Exception $e) {
return false;
}
}

public function method_json_encode_multi_flags(string $json): string | bool
{
try {
json_encode($json, JSON_PRETTY_PRINT);
if (json_last_error() !== JSON_ERROR_NONE) {
throw new \Exception(json_last_error_msg());
}
} catch (\Exception $e) {
return false;
}
}
}

// Function
function func_json_encode(string $json): string | bool
{
try {
json_encode($json, 0);
if (json_last_error() !== JSON_ERROR_NONE) {
throw new \Exception(json_last_error_msg());
}
} catch (\Exception $e) {
return false;
}
}

// Standalone
json_encode($json, 0);
if (json_last_error() !== JSON_ERROR_NONE) {
throw new \Exception(json_last_error_msg());
}

?>
3 changes: 3 additions & 0 deletions ...DowngradePhp73/Rector/ConstFetch/DowngradePhp73JsonConstRector/Fixture/in_bitwise.php.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ class InBitwise
public function run($argument)
{
$argument = json_encode($argument, JSON_UNESCAPED_SLASHES);
if (json_last_error() !== JSON_ERROR_NONE) {
throw new \Exception(json_last_error_msg());
}
}
}

Expand Down
Loading
Loading