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.

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

[Rector] Apply Full PHP 7.3 Rector Set List (Skip JsonThrowOnErrorRector & StringifyStrNeedlesRector) #4601

Conversation

samsonasik
Copy link
Member

@samsonasik samsonasik commented Apr 23, 2021

Alternative of #4600 with skip JsonThrowOnError & StringifyStrNeedlesRector.

Checklist:

  • Securely signed commits

Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

I don't understand why the string cast is needed here.

@samsonasik
Copy link
Member Author

That's based on the following https://wiki.php.net/rfc/deprecations_php_7_3#string_search_functions_with_integer_needle

For function that return string from helper, eg, singular(), the helpers directory seems need to be registered to Option::BOOTSTRAP_FILES to make skip function that already return string to no need re-cast.

@samsonasik
Copy link
Member Author

We can skip StringifyStrNeedlesRector if it is unneeded check.

@paulbalandan
Copy link
Member

Looking at the previous lines where the string casted variables arise, it seems these are already string so casting is unnecessary.

@samsonasik samsonasik force-pushed the apply-rector-php73-feature-alternative branch from 39a6a06 to c1c047a Compare April 23, 2021 07:36
@samsonasik
Copy link
Member Author

Ok, I skipped the StringifyStrNeedlesRector as well.

@samsonasik samsonasik changed the title [Rector] Apply Full PHP 7.3 Rector Set List (Skip JsonThrowOnErrorRector) [Rector] Apply Full PHP 7.3 Rector Set List (Skip JsonThrowOnErrorRector & StringifyStrNeedlesRector) Apr 23, 2021
Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

Seems this is the better choice for now.

Just wondering, what are in the Set List for 7.3?

@samsonasik
Copy link
Member Author

That's collection of rector rules for php 7.3, you can check at:
https://github.com/rectorphp/rector/blob/main/config/set/php73.php

@samsonasik
Copy link
Member Author

I am merging it then, thank you @paulbalandan @MGatner for the review.

@samsonasik samsonasik merged commit f66c545 into codeigniter4:develop Apr 24, 2021
@samsonasik samsonasik deleted the apply-rector-php73-feature-alternative branch April 24, 2021 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants