-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Update phpcpd to ~6.0.3 to be PHP8 compatible #32196
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
Update phpcpd to ~6.0.3 to be PHP8 compatible #32196
Conversation
Hi @hws47a. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
You can find more information about the builds here ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review. For more details, please, review the Magento Contributor Guide documentation. 🕙 You can find the schedule on the Magento Community Calendar page. 📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket. 🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel ✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel |
@sivaschenko Static Tests passed 👏 Also I've run static tests locally and phpcpd worked fine. |
@sivaschenko some integration test has failed for Integration B2B but I cannot find it in partners-b2b repository and php8-develop branch is not available there. Could you help me with this? |
@magento run Integration Tests |
@ihor-sviziev it seems that all passed |
@hws47a, could you explain why we need to update PHPUnit in the scope of this PR? |
You may see that in composer.json only phpcpd was changed and PHPUnit was allowed to be any ^9 version. So a bit newer PHPUnit is a requirement for phpcpd 6.0.3 |
@hws47a thanks for the update, can you please also update the composer.json and lock in EE repo and link the EE PR as related |
@hws47a btw, the php8-develop should be present for all repositories, i.e here is B2B branch https://github.com/magento/partners-magento2b2b/tree/php8-develop |
Done |
@magento run Functional Tests B2B, Static Tests |
@hws47a, static tests are still failing. Could you check, please? |
172fde2
to
e2ca21a
Compare
@sivaschenko any thoughts why tests are failing with no build results? |
@hws47a @ihor-sviziev tests have failed due to conflict in the EE PR |
@magento run all tests |
1 similar comment
@magento run all tests |
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.
Could you resolve conflicts in EE repo and run all tests again?
@magento run Functional Tests B2B, Functional Tests EE, Static Tests |
1 similar comment
@magento run Functional Tests B2B, Functional Tests EE, Static Tests |
@magento run Static Tests |
@magento run Static Tests |
1 similar comment
@magento run Static Tests |
@magento run all tests |
Hi @sivaschenko, I've fixed phpcpd locally including ee and b2b, however it still failing in the pipeline. I've added output of the command and you may see in the pipeline that there is no output at all. So it seems like the command doesn't run a all. Could you please check it? |
Let's try to re-run the tests |
@magento run Static Tests |
Executing PHPCPD in the phpunit process for debugging purpose @magento run Static Tests |
@hws47a looks like phpcpd fails to compare bitmap files also it does find a number of copy-pastes, mostly in Inventory modules |
@magento run Static Tests |
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.
Please review my comments
dev/tests/static/framework/Magento/TestFramework/CodingStandard/Tool/CopyPasteDetector.php
Outdated
Show resolved
Hide resolved
$exclude[] = $files; | ||
} | ||
|
||
return array_unique(array_merge(...$exclude)); |
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.
Technically, there might be empty array in the $exclude, so it's better to replace it with return array_unique(array_merge([], ...$exclude));
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.
I've tried in interactive shell and it seems that array_merge(...[])
doesn't throw any warnings and just returns an empty array. Do you see some PHP error while testing?
10a5700
to
6cf8a8b
Compare
6cf8a8b
to
5b7bf7e
Compare
Changed implementation of |
@magento run all tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
@magento run all tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time. |
Hi @hws47a, thank you for your contribution! |
Description (*)
Update phpcpd to ~6.0.3 with minimal changes to other dependencies including update phpunit/php-timer from ^3.0 to ^5.0 and phpunit from 9.1 to 9.2
Changes included:
Related Pull Requests
https://github.com/magento/partners-magento2ee/pull/501
https://github.com/magento/partners-magento2b2b/pull/563
Fixed Issues (if relevant)
Contribution checklist (*)