-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add Rector in archive creation #135
Add Rector in archive creation #135
Conversation
Signed-off-by: Alex Pantechovskis <[email protected]>
Signed-off-by: Alex Pantechovskis <[email protected]>
Signed-off-by: Alex Pantechovskis <[email protected]>
Signed-off-by: Alex Pantechovskis <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the use case of Rector? Isn't that something you only execute once and when the project is upgraded you can remove it? 👀 Right now, it seems that code is changed on the fly, but never tested before creating a release. So you build a release on something untested?
There are other PRs with workflows for testing the archive. :) (+ hopefully Playwright in the future) For transpiling the idea is that we use the latest PHP for development, and then transform into 7.4 for release. |
8a3610c
to
4d62458
Compare
4d62458
to
3ba2563
Compare
This here is a build/packaging workflow, not a release workflow. The produced builds are a prerequisite for further (automated&manual) testing by QA. |
Signed-off-by: Alex Pantechovskis <[email protected]>
Signed-off-by: Alex Pantechovskis <[email protected]>
Signed-off-by: Alex Pantechovskis <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this. While the changes LGTM code-wise, the addition of Rector to the reusable workflows can only be temporary until all products require modern PHP versions natively, IMO.
Left two minor comments that don't stop me from approving. Thanks!
Co-authored-by: Philipp Bammes <[email protected]> Signed-off-by: Alex Pantechovskis <[email protected]>
Co-authored-by: Philipp Bammes <[email protected]> Signed-off-by: Alex Pantechovskis <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for incorporating the changes!
I think products will never use only the latest version unless something greatly changes in the WP/PHP world. We will probably still need to support PHP 8 when PHP 9 will be available for years because many merchants are slow to upgrade. Also Rector is quite generic tool, it just transforms the code via AST using the specified rules, so maybe we find some other use cases for it. |
Please check if the PR fulfills these requirements
What kind of change does this PR introduce?
feature
Now the package creation workflow also runs Rector if the config is present. This was needed for researching PHP 8 transpiling into PHP 7.4, but also can be useful for other tasks.
Increased the timeout to 10 minutes because sometimes it takes a bit more than 5 minutes. (Rector takes around 40 seconds in the WL project)