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

Fix: Keep packages sorted #10769

Merged
merged 1 commit into from
Oct 24, 2017
Merged

Conversation

localheinz
Copy link
Contributor

@localheinz localheinz commented Sep 4, 2017

Description

By configuring the sort-packages option in composer.json

  • requiring or
  • removing

a dependency will automatically trigger sorting of the packages in the corresponding

  • require
  • require-dev

sections.

Of course, as an alternative to configuring this in composer.json, the --sort-packages flag can be used manually.

Keeping these packages sorted has the following advantages:

  • it's easier to scan a list for items when it is sorted
  • due to the nature of sorting, the more important platform requirements will rise to the top

Since there is no command for adding or removing items to or from the replace section, I sorted those manually using the Lines Sorter plugin for PhpStorm.

💁‍♂️ For reference, see

Fixed Issues (if relevant)

n/a

Manual testing scenarios

n/a

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)
  • All automated tests passed successfully (all builds on Travis CI are green)This PR

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Sep 4, 2017

CLA assistant check
All committers have signed the CLA.

composer.json Outdated
@@ -7,80 +7,83 @@
"OSL-3.0",
"AFL-3.0"
],
"config": {
"sort-packages": true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's what I ran after adding this section to trigger sorting packages (without updating any dependencies):

$ composer require composer/composer:1.4.1
$ composer require --dev squizlabs/php_codesniffer:3.0.1

composer.json Outdated
},
"replace": {
"magento/module-marketplace": "100.3.0-dev",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's what I did after sorting packages in this section using https://plugins.jetbrains.com/plugin/5919-lines-sorter:

  • ran composer validate
  • fixed the trailing comma issue
  • ran composer update --lock

For reference, see:

@localheinz
Copy link
Contributor Author

There's quite a bunch of composer.json checked in, should the change be applied to all of them?

@okorshenko okorshenko self-assigned this Sep 6, 2017
@okorshenko okorshenko added this to the September 2017 milestone Sep 6, 2017
@okorshenko okorshenko removed their assignment Oct 1, 2017
@dmanners
Copy link
Contributor

dmanners commented Oct 5, 2017

@localheinz if you feel up for it then you could update all the composer.json files that are checked in.

@dmanners
Copy link
Contributor

Thanks for the update @localheinz could you take a moment to look into the conflict on the file app/code/Magento/Backend/composer.json for me. Thank you.

@localheinz
Copy link
Contributor Author

@dmanners

Done!

Copy link
Contributor

@dmanners dmanners left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of syntax errors, but other than that it looks good.

"magento/magento2-functional-testing-framework": "dev-develop"
"vlucas/phpdotenv": "~2.4",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be a miss , the line above.

"magento/magento2-functional-testing-framework": "dev-develop"
"vlucas/phpdotenv": "~2.4",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be a miss , the line above.

"magento/module-quote": "100.3.*"
"magento/module-sales": "100.3.*",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be a miss , the line above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for catching these, @dmanners!

Copy link
Contributor

@dmanners dmanners left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good. Will see what the builds say and let you know.

Copy link
Contributor

@dmanners dmanners left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I am going to run this through our internal systems.

@magento-engcom-team magento-engcom-team changed the base branch from develop to 2.3-develop October 20, 2017 15:32
@okorshenko okorshenko merged commit f24b03f into magento:2.3-develop Oct 24, 2017
okorshenko pushed a commit that referenced this pull request Oct 24, 2017
@localheinz localheinz deleted the fix/sort-packages branch October 24, 2017 06:02
@localheinz
Copy link
Contributor Author

Thank you, @dmanners and @okorshenko!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants