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

[4.x]: The update command modifies the package order in the require section #13806

Closed
MoritzLost opened this issue Oct 10, 2023 · 7 comments
Closed

Comments

@MoritzLost
Copy link
Contributor

MoritzLost commented Oct 10, 2023

What happened?

Description

I'm seeing an error where running php craft update all changes the order of requirements in the composer.json. Before:

"require": {
    "php": "~8.2.0",
    "clubstudioltd/craft-asset-rev": "7.0.0",
    "craftcms/ckeditor": "3.6.0",
    "craftcms/cms": "4.5.5",
    "craftcms/contact-form": "3.0.1",
    "rynpsc/craft-phone-number": "2.1.0",
    "sebastianlenz/linkfield": "2.1.5",
    "twig/string-extra": "^3.5",
    "verbb/navigation": "2.0.21",
    "verbb/smith": "2.0.0",
    "verbb/super-table": "3.0.9",
    "vlucas/phpdotenv": "^5.5.0"
},

After:

"require": {
    "clubstudioltd/craft-asset-rev": "7.0.0",
    "craftcms/ckeditor": "3.6.0",
    "craftcms/cms": "4.5.6.1",
    "craftcms/contact-form": "3.0.1",
    "php": "~8.2.0",
    "rynpsc/craft-phone-number": "2.2.0",
    "sebastianlenz/linkfield": "2.1.5",
    "twig/string-extra": "^3.5",
    "verbb/navigation": "2.0.21",
    "verbb/smith": "2.0.0",
    "verbb/super-table": "3.0.12",
    "vlucas/phpdotenv": "^5.5.0"
},

I don't like this behaviour. We prefer to have platform requirements (in particular, the php requirement) at the top to see which PHP version a project is using at a glance. This seems to be a behaviour of Craft's update in particular. Running composer update -W (after changing the fixed version numbers to version ranges) doesn't exhibit this behaviour.


As a side note, it also looks like it's stripping the trailing newline at the end of the file. Composer retains the newline if it exists, and many editors are configured to add it back if it's missing, so this results in some unnecessary noise in git diffs.

Steps to reproduce

  1. Put a php requirement at the top of the require section in your composer.json.
  2. Run php craft update all in a project with pending Craft/plugin updates.

Expected behavior

The command should never modify the order of packages in the requirement. If this is necessary for implementation reasons, it should at least put system requirements (like the php requirement) at the top so they aren't mixed in with other packages.

Actual behavior

The order of packages is changed, looks like it's sorting them alphabetically. This might make sense for normal package, but nor for system requirements.

Craft CMS version

4.5.5

PHP version

8.2

Operating system and version

macOS

Database type and version

No response

Image driver and version

No response

Installed plugins and versions

No response

@brandonkelly
Copy link
Member

brandonkelly commented Oct 10, 2023

Do you have config.sort-packages set to true in composer.json?

@MoritzLost
Copy link
Contributor Author

@brandonkelly D'oh, I'm stupid. You're right, totally forgot that we had that in there. Removed it and now the order is retained. This behaviour just surprised me, since this didn't use to happen in previous Craft versions – did that change with the recent refactoring of the Composer integration?

@brandonkelly
Copy link
Member

We’ve been passing the sort-packages value off to Composer since 3.0.0:

!$manipulator->addLink($requireKey, $package, $constraint, $sortPackages) ||

However it’s possible that we are enforcing it more strictly than Composer‘s internal JSON manipulation logic does, now that we’re no longer using Composer\Json\JsonManipulator in Craft 4.5. (Maybe Composer only enforces it when requiring new packages or something.)

That said, it feels like expected behavior that if you have sort-packages set to true, in general Craft should be respecting it and keeping your packages sorted alphabetically whenever it’s making changes to them, even if just updating their versions.

@MoritzLost
Copy link
Contributor Author

@brandonkelly Yes, this is absolutely the expected behaviour! I was just caught off guard. I've noticed that the Craft commands behave slightly differently than native Composer commands (e.g. it removes the newline at the end if there is one, while Composer keeps this as-is). But yeah, having sort-packages work like it does for Composer commands is definitely correct. I wish there was a config option to sort required libraries alphabetically, but keep system packages at the top. But I guess that's a feature request for Composer itself, not Craft :)

@brandonkelly
Copy link
Member

Actually, looks like that’s exactly what JsonManipulator is doing. Just fixed Craft’s own package sorting to be consistent with that. So now php will stay at the top, followed by php-*, hhvm, ext-*, and lib-*.

@MoritzLost
Copy link
Contributor Author

@brandonkelly Awesome, thanks Brandon! That looks great, didn't realize Composer behaved like this already. Very nice that Craft matches this behaviour now.

@brandonkelly
Copy link
Member

Craft 4.5.7 is out now with that fix 🎉

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

No branches or pull requests

2 participants