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.

Sign up for GitHub

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

Jump to bottom

Removed a fixed version phpcs dependency from the project #35807

Merged
merged 3 commits into from
Dec 3, 2022
Merged

Removed a fixed version phpcs dependency from the project #35807

merged 3 commits into from
Dec 3, 2022

Conversation

bgorski
Copy link
Contributor

@bgorski bgorski commented Jul 23, 2022 •
edited by m2-assistant bot
Loading

Description (*)

This Pull Request removes a PHP Code Sniffer fixed version dependency from the project's main composer.json file.
The reason for that is for purposes of maintaining a Magento 2 project's code quality, phpcs alone is almost useless. M2 has its own phpcs coding standard, which should be used, and which (obviously) also lists phpcs among its dependencies. The coding standard is also included in the project's main composer.json file so can be considered an integral part of the platform. The coding standard is also the only part of the platform that developers working on Magento projects will use phpcs with.
We should also assume that if any Magento 2 project should use a different coding standard, that coding standard will also list phpcs among its dependencies.
Therefore, the conclusion is that Magento2 itself should NOT manage PHP Code Sniffer's version. It should only be managed by the installed coding standard.

At the point of writing this text, if you remove phpcs from composer.json manually and you run the following command:
composer require magento/magento-coding-standard --dev --ignore-platform-reqs --with-dependencies
The output will contain this:

  • Upgrading squizlabs/php_codesniffer (3.6.2 => 3.7.1)

This means that - for no reason - the main composer.json file prohibits the coding standard from using the latest phpcs version with which it was already marked as compatible.

Additionally, keeping that version there limits the extensibility. For example, if you choose to use the MRM Coding Standard (which also requires and uses the Magento Coding Standard), you need to manually remove the phpcs from the main composer.json before you run the following command:
composer require mrm-commerce/magento-coding-standard-phpcs:3.0.0-RC1 --dev --ignore-platform-reqs --with-dependencies
That's because it requires phpcs version to be at least 3.7.1, which is compatible with Magento Coding Standard, but not with the version required by the main composer.json.

So again, there is no reason for limiting 3rd party coding standards from being used when they're 100% compatible with the Magento Coding Standard.

Manual testing scenarios (*)

  1. Remove "squizlabs/php_codesniffer" from the "require-dev" part of composer json manually
  2. Run composer require magento/magento-coding-standard --dev --ignore-platform-reqs --with-dependencies
  3. Access composer.lock
  4. Notice that squizlabs/php_codesniffer is there, and even in a newer version

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
  • All automated tests passed successfully (all builds are green)

Resolved issues:

  1. resolves [Issue] Removed a fixed version phpcs dependency from the project #35994: Removed a fixed version phpcs dependency from the project

@m2-assistant
Copy link

m2-assistant bot commented Jul 23, 2022

Hi @bgorski. Thank you for your contribution
Here are some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s)
    For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE,
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests
  13. Semantic Version Checker

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.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 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

@m2-github-services m2-github-services added Partner: MRM Commerce partners-contribution Pull Request is created by Magento Partner labels Jul 23, 2022
@m2-community-project m2-community-project bot added this to Pending Review in Pull Requests Dashboard Jul 23, 2022
@bgorski
Copy link
Contributor Author

bgorski commented Jul 23, 2022

@magento run all tests

@magento-automated-testing
Copy link

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.

@bgorski
Copy link
Contributor Author

bgorski commented Jul 25, 2022

@magento run all tests

@magento-automated-testing
Copy link

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.

@bgorski
Copy link
Contributor Author

bgorski commented Jul 26, 2022

@magento run Functional Tests B2B, Functional Tests EE, Functional Tests CE

@magento-automated-testing
Copy link

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.

@sidolov sidolov added the Priority: P2 A defect with this priority could have functionality issues which are not to expectations. label Jul 26, 2022
@m2-community-project m2-community-project bot removed this from Pending Review in Pull Requests Dashboard Jul 26, 2022
@m2-community-project m2-community-project bot moved this from Pending Review to Ready for Testing in High Priority Pull Requests Dashboard Aug 17, 2022
@engcom-Lima engcom-Lima self-assigned this Aug 22, 2022
@engcom-Lima engcom-Lima moved this from Ready for Testing to Testing in Progress in High Priority Pull Requests Dashboard Aug 22, 2022
@engcom-Lima
Copy link
Contributor

@magento create issue

@m2-assistant m2-assistant bot mentioned this pull request Aug 22, 2022
Closed
5 tasks
@m2-community-project m2-community-project bot moved this from Testing in Progress to Ready for Testing in High Priority Pull Requests Dashboard Aug 22, 2022
@engcom-Lima engcom-Lima moved this from Ready for Testing to Testing in Progress in High Priority Pull Requests Dashboard Aug 22, 2022
@engcom-Echo engcom-Echo moved this from Ready for Testing to Extended Testing (optional) in High Priority Pull Requests Dashboard Nov 23, 2022
@engcom-Echo
Copy link
Contributor

@magento run all tests

@magento-automated-testing
Copy link

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.

@engcom-Echo
Copy link
Contributor

@magento run Functional Tests EE, Functional Tests B2B

@magento-automated-testing
Copy link

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.

1 similar comment
@magento-automated-testing
Copy link

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.

@engcom-Echo
Copy link
Contributor

@magento run Functional Tests B2B

@magento-automated-testing
Copy link

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.

1 similar comment
@magento-automated-testing
Copy link

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.

@engcom-Echo
Copy link
Contributor

@magento run Functional Tests EE, Functional Tests B2B

@magento-automated-testing
Copy link

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.

@abhattGlo
Copy link
Contributor

@magento run Functional Tests EE, Functional Tests B2B

@magento-automated-testing
Copy link

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.

1 similar comment
@magento-automated-testing
Copy link

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.

@engcom-Echo
Copy link
Contributor

@magento run Functional Tests B2B

@magento-automated-testing
Copy link

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.

@engcom-Echo
Copy link
Contributor

@magento run all tests

@magento-automated-testing
Copy link

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.

@engcom-Echo
Copy link
Contributor

engcom-Echo commented Dec 2, 2022 •
edited
Loading

Funtional B2B and EE failure are not related to PR.
As on last three run for EE and B2B different test failed.

Run 1

EE
image

B2B
image

Run 2
EE
image

B2B
image

Run 3
EE
image

B2B
image

Hence Moving to merge in progress

@engcom-Echo engcom-Echo moved this from Extended Testing (optional) to Merge in Progress in High Priority Pull Requests Dashboard Dec 2, 2022
@magento-devops-reposync-svc magento-devops-reposync-svc merged commit b2cbe0b into magento:2.4-develop Dec 3, 2022
@ishakhsuvarov ishakhsuvarov moved this from Merge in Progress to Recently Merged in High Priority Pull Requests Dashboard Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ihor-sviziev ihor-sviziev ihor-sviziev approved these changes

Assignees

@ihor-sviziev ihor-sviziev

@engcom-Echo engcom-Echo

@engcom-Lima engcom-Lima

Labels
Partner: MRM Commerce partners-contribution Pull Request is created by Magento Partner Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: accept
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Issue] Removed a fixed version phpcs dependency from the project
9 participants
@bgorski @engcom-Lima @ishakhsuvarov @engcom-Echo @abhattGlo @ihor-sviziev @sidolov @magento-devops-reposync-svc @m2-github-services