-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Upgrade phpcodesniffer-composer-installer #36791
Conversation
Hi @fredden. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
You can find more information about the builds here ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review. For more details, 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, join the Community Contributions Triage session to discuss the appropriate ticket. ✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel |
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. |
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.
Hi @fredden Thanks for your update.
Please review my comment
@@ -90,7 +90,7 @@ | |||
}, | |||
"require-dev": { | |||
"allure-framework/allure-phpunit": "^2", | |||
"dealerdirect/phpcodesniffer-composer-installer": "^0.7", | |||
"dealerdirect/phpcodesniffer-composer-installer": "^0.7 || ^1.0", |
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.
As same as for codding-standard repo PR I believe that "^1.0"
will be enough
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.
There's nothing here that's not compatible with ^0.7.0
. I've specifically kept this because I expect other packages may take a while to upgrade / allow the new version. If there's something that requires ^0.7
, it'll work today, but not after merging if we change this to ^1.0
only. Listing both supported versions means that interoperability with other packages is maintained; Composer will pick the "best" version, which is typically the highest mutually-supported version. If we use ^1.0
only here, then we're forcing others to upgrade their packages in order for their coding standard to be used in conjunction with this product.
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.
Make sense
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.
Failed test is not related to changes
@magento run Functional Tests B2B |
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. |
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. |
As per above comment, moving it to Merge In Progress. Thank you! |
@magento create issue |
@engcom-Charlie, Adobe needs to decide if they want to accept the breaking changes introduced by @glo71317 in d337684 (in which case this pull request should be closed), or not (in which case this pull request should be merged as was the plan since 15th February 2023). |
Hi @fredden, i don't think there is any breaking changes, And also not forcing to any other vendor and customer to update the dependency in current magento version which they are using they can continue without any breaking changes, As per my understanding If they will upgrade with current version to upcoming 2.4.7-beta2 they can do without any breaking changes. |
I have already articulated this. Please see #36791 (comment)
|
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues. |
@glo30253 I'm agree with @fredden |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues. |
@fredden @Den4ik As per discussion with PO and Engineering Manager. @Den4ik please update the pr based on above discussion if you want to deliver these changes else we will do and cancel this PR? |
@glo71317 you introduced the breaking change in d337684. This pull request fixes the breaking part of that commit by re-introducing support for versions |
@fredden @Den4ik Okay make sense thanks for your contribution. |
@glo71317 Great, thanks for conversation |
@magento run Functional Tests B2B, Functional Tests CE, Functional Tests EE, WebAPI Tests |
The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues. |
As the this comment, and as the PR is already been tested prior hence moving this PR to Merge in Progress. |
This PR got merged in a2519ca hence closing this now. |
@engcom-Charlie: please link to the publicly visible repo, it should be this link: a2519ca |
Description
dealerdirect/phpcodesniffer-composer-installer
has now reached its first stable version. See https://github.com/PHPCSStandards/composer-installer/releases/tag/v1.0.0 for details.Manual testing scenarios
There are no functional changes between these two versions.
Contribution checklist
Resolved issues: