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

refactor: upgrade to PHP 8.1 with rector #7964

Closed
wants to merge 21 commits into from

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Sep 23, 2023

Description
Supersedes #6923
See #6921 and #8042

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added refactor Pull requests that refactor code 4.5 labels Sep 23, 2023
@kenjis kenjis mentioned this pull request Sep 23, 2023
4 tasks
@kenjis
Copy link
Member Author

kenjis commented Sep 23, 2023

@samsonasik When I ran rector first, the following change did not happen.
I ran rector again, I got the change.

$ vendor/bin/rector 
 119/119 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
1 file with changes
===================

1) system/Router/AutoRouterImproved.php:410

    ---------- begin diff ----------
@@ @@
     {
         try {
             $refClass = new ReflectionClass($this->controller);
-        } catch (ReflectionException $e) {
+        } catch (ReflectionException) {
             throw PageNotFoundException::forControllerNotFound($this->controller, $this->method);
         }
    ----------- end diff -----------

Applied rules:
 * RemoveUnusedVariableInCatchRector (https://wiki.php.net/rfc/non-capturing_catches)

@samsonasik
Copy link
Member

@kenjis rector sometime need twice run as the code got consecutive run on single rule or just reprinted on next rule apply to specific node to avoid infinite loop

@kenjis
Copy link
Member Author

kenjis commented Sep 23, 2023

@samsonasik I got it. Thanks!

system/View/Cells/Cell.php Outdated Show resolved Hide resolved
@kenjis
Copy link
Member Author

kenjis commented Oct 6, 2023

Rebased and re-run rector.

@MGatner
Copy link
Member

MGatner commented Oct 10, 2023

I'm not going to review all the changes, but what I spot-checked looks good and pipeline is passing.

This will inevitably be a big merge conflict before we are ready for the next minor release. I suggest you get the automated changes in place via Rector (done) then consolidate the manual revisions to a commit(s) that can be cherry-picked after rerunning Rector.

Excited to be on 8+! Thanks for your work on this - and shoutout to the attention from @TomasVotruba 😍 and the tireless effort from the whole Rector team, including our own @samsonasik.

@kenjis
Copy link
Member Author

kenjis commented Oct 10, 2023

@MGatner I don't understand what you exactly mean.
I did atomic commits. Do you suggest to combine the commit for refactor by Rector and the commits for manual tweaks into one commit?

@MGatner
Copy link
Member

MGatner commented Oct 12, 2023

No, sorry to be unclear! I think what you are doing is great - I was just noting that it might not be worthwhile to keep up with conflicts on this PR, but rather wait and rerun Rector right before we are ready for release.

@kenjis kenjis marked this pull request as draft October 12, 2023 23:11
@kenjis
Copy link
Member Author

kenjis commented Oct 12, 2023

@MGatner I got your point. Thanks.

@github-actions github-actions bot added the stale Pull requests with conflicts label Oct 16, 2023
@github-actions
Copy link

👋 Hi, @kenjis!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

@kenjis kenjis removed the stale Pull requests with conflicts label Oct 16, 2023
@kenjis kenjis force-pushed the rector-upgrade-php80-CI45 branch 2 times, most recently from 7ceb7e4 to 4366d27 Compare October 18, 2023 02:00
@kenjis kenjis removed the stale Pull requests with conflicts label Nov 29, 2023
@github-actions github-actions bot added the stale Pull requests with conflicts label Dec 6, 2023
Copy link

github-actions bot commented Dec 6, 2023

👋 Hi, @kenjis!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

@kenjis
Copy link
Member Author

kenjis commented Dec 21, 2023

Due to the difficulty in resolving the conflicts, I close this PR.
Go to #8354.

@kenjis kenjis closed this Dec 21, 2023
@kenjis kenjis mentioned this pull request Feb 3, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.5 refactor Pull requests that refactor code stale Pull requests with conflicts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants