Page MenuHomePhabricator

CheckComposerLockUpToDate / version check is incorrect and causes "Error: your composer.lock file is not up to date. ..."
Closed, DeclinedPublic

Description

Using ^ or ~ as version constraint is wrongfully marked by CheckComposerLockUpToDate as invalid dependency.

$ php maintenance/update.php
MediaWiki 1.29.1 Updater
mediawiki/semantic-media-wiki: 2.5.4 installed, ~2.5 required.
Error: your composer.lock file is not up to date. Run "composer update" to install newer dependencies

The check $installed[$name]['version'] !== $version does not take into account a ^ or ~ as constraint which then obviously fails on comparing 2.5.4 to ~2.5.

		$installed = $lock->getInstalledDependencies();
		foreach ( $json->getRequiredDependencies() as $name => $version ) {
			if ( isset( $installed[$name] ) ) {
				if ( $installed[$name]['version'] !== $version ) {

Please use [0] as reference when comparing and validate version constraints.

[0] https://getcomposer.org/doc/articles/versions.md

Event Timeline

mwjames created this task.Oct 18 2017, 6:44 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 18 2017, 6:44 AM

@Legoktm If time permits please comment on the outlined issue.

Is mediawiki/semantic-media-wiki in $IP/composer.json? It should only be in composer.local.json...

Kghbln added a subscriber: Kghbln.Oct 22 2017, 2:59 PM

It should only be in "composer.local.json"

Reality has it, not saying that I do this, that a lot of people not too familiar with MediaWiki are not aware of "composer.local.json" which appears to be specific to MediaWiki so instinctively they use "composer.json". So I think it will be nice to fix this instead of trying to defy reality.

It should only be in "composer.local.json"

Reality has it, not saying that I do this, that a lot of people not too familiar with MediaWiki are not aware of "composer.local.json" which appears to be specific to MediaWiki so instinctively they use "composer.json". So I think it will be nice to fix this instead of trying to defy reality.

If people are using composer.json that's problematic for other reasons besides this check since their changes are going to get overwritten on every upgrade. Do we need to advertise composer.local.json more prominently in documentation? e.g. in https://github.com/SemanticMediaWiki/SemanticMediaWiki/blob/master/docs/INSTALL.md#step-2 it advertises the old, pre-1.25 way before the modern method.

Kghbln added a comment.EditedOct 23 2017, 10:26 PM

Do we need to advertise composer.local.json more prominently in documentation? e.g. in https://github.com/SemanticMediaWiki/SemanticMediaWiki/blob/master/docs/INSTALL.md#step-2 it advertises the old, pre-1.25 way before the modern method.

I think so. About the mentioned INSTALL.md: As a matter of fact this section will go entirely with the upcoming 3.0.0 release of SMW. This info is legacy and was only kept due to SMW still supporting now obsolete versions of MW.

Still we are currently discussing about how to fix the user instead of fixing the software. The latter can be problematic the former is definitively problematic. Currently I have the impression that we agree that there is an issue with the software but we do not agree if it should be fixed.

Do we need to advertise composer.local.json more prominently in documentation? e.g. in

This is social engineering problem that can be addressed one way or the other.

The reported problem (as outlined in the initial report) is an engineering task which caused by an insufficiency in the software and I think that part should be addressed independently of the discussion about composer.local.json.

Legoktm closed this task as Declined.Nov 4 2017, 6:46 PM

From my POV the script is working as it is expected to - we only used fixed versions in MediaWiki core's composer.json (which is why the script only supports fixed versions), and people should not be putting other dependencies in there, like mediawiki/semantic-media-wiki - that is problematic. Instead composer.local.json should be used, and I'm happy to discuss how to make that process better.

From my POV the script is working as it is expected to - we only used fixed versions in MediaWiki core's composer.json

Is that an official policy? Is that documented somewhere? Is this is an official WMF restriction or a general MediaWiki restriction? What are the arguments for the artificial restriction since there is no programmatically (ie technical) limitation on allowing ~, ^ as to what is supported by the official Composer specification.

From my POV the script is working as it is expected to - we only used fixed versions in MediaWiki core's composer.json

Is that an official policy? Is that documented somewhere? Is this is an official WMF restriction or a general MediaWiki restriction? What are the arguments for the artificial restriction since there is no programmatically (ie technical) limitation on allowing ~, ^ as to what is supported by the official Composer specification.

It's documented at https://www.mediawiki.org/wiki/Manual:External_libraries and has been the MediaWiki core policy since we started using external libraries in 1.25. I've discussed this on wikitech-l a few times, but briefly the reasoning is that not all maintainers actually follow semver despite claiming to do so, and that composer.lock is not a usable solution while still allowing extension dependencies (which is why composer-merge-plugin exists).

ahmad added a subscriber: ahmad.Jan 2 2018, 11:43 AM

From my POV the script is working as it is expected to

Please consider adding a hook, so non WMF extensions can decide freely on which approach they follow without the need to consider a POV argument.

$installed = $lock->getInstalledDependencies();
foreach ( $json->getRequiredDependencies() as $name => $version ) {

	if ( !Hooks::run( 'ComposerLock::CheckDependency', array( $name, $version ) ) ) {
		continue;
	};

From my POV the script is working as it is expected to

Please consider adding a hook, so non WMF extensions can decide freely on which approach they follow without the need to consider a POV argument.

I'm not sure what you mean, as extensions aren't supposed to be adding things to core's composer.json.

I'm not sure what you mean, as extensions aren't supposed to be adding things to core's composer.json.

Well, that is one position and as outlined to avoid POV discussion over what extensions are allowed or suppose to (in the framework of WMF centred position), hooks enable extension developers to make their own decisions without considering some higher order policy.

Reason is that that common users don't grasp the difference between composer.local.json and composer.json (and they should not) and I now and then receive emails with questions (and rants about the incapacity to provide a simple system which it has been before someone made the mentioned changes), I'm honestly tired of the constant explaining and pointing to task like this one and cleaning my inbox on issues where the cause is due to changes in policy which lies outside of third-party involvement.

I'm not sure what you mean, as extensions aren't supposed to be adding things to core's composer.json.

Well, that is one position and as outlined to avoid POV discussion over what extensions are allowed or suppose to (in the framework of WMF centred position), hooks enable extension developers to make their own decisions without considering some higher order policy.

We're going in circles here. 3 months ago you asked if this was an "official policy", so I gave you the link, and explained. Now two months later you're deciding to characterize this as "one position", despite it being a generally accepted MediaWiki core policy. On top of that there's also https://www.mediawiki.org/wiki/Do_not_hack_MediaWiki_core#Why_you_should_not_modify_core_files and so on.

Reason is that that common users don't grasp the difference between composer.local.json and composer.json (and they should not) and I now and then receive emails with questions (and rants about the incapacity to provide a simple system which it has been before someone made the mentioned changes), I'm honestly tired of the constant explaining and pointing to task like this one and cleaning my inbox on issues where the cause is due to changes in policy which lies outside of third-party involvement.

In November, you said this was a social engineering problem. Now you're saying that users shouldn't know the difference? Please clarify :)