Page MenuHomePhabricator

extension.json should be able to express dependency on PHP version / PHP extension version
Closed, ResolvedPublic

Description

A MediaWiki extension might depend on a specific PHP extension being installed (maybe even on a PHP version). Composer can check for these but there is no way to express them in extension.json.

Event Timeline

Tgr created this task.Jun 17 2018, 10:02 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 17 2018, 10:02 AM

Sounds like a good idea to me, I've been thinking about something similar as well.

	"requires": {
		"MediaWiki": ">= 1.31.0",
		"platform": {
			"php": ">=7.0.0",
			"ext-foo": "*",
			"ability-multihttp": true,
			"ability-shell": true
		}
	},

It gets nested under the "platform" key, you can depend upon PHP version (php), any PHP extensions (ext-whatever), and a list of "abilities": multihttp -> MultiHttpClient (e.g. VE), shell -> !Shell::isDisabled() (e.g. Score).

Vvjjkkii renamed this task from extension.json should be able to express dependency on PHP version / PHP extension version to 8raaaaaaaa.Jul 1 2018, 1:03 AM
Vvjjkkii triaged this task as High priority.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed a subscriber: Aklapper.
CommunityTechBot renamed this task from 8raaaaaaaa to extension.json should be able to express dependency on PHP version / PHP extension version.Jul 2 2018, 4:44 AM
CommunityTechBot raised the priority of this task from High to Needs Triage.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added a subscriber: Aklapper.

Change 458940 had a related patch set uploaded (by MGChecker; owner: MGChecker):
[mediawiki/core@master] [WIP] Add possibility to require a given PHP version

https://gerrit.wikimedia.org/r/458940

MGChecker claimed this task.Sep 9 2018, 1:50 PM
MGChecker added a subscriber: MGChecker.

I will start with the php key, maybe adding the ext- keys as well.

MGChecker added a comment.EditedSep 10 2018, 11:43 AM

PHP extensions are kind of weird. It's easy to check if they're loaded at all ( get_loaded_extensions() ), and there is phpversion( $extension ) to check for their version. However, it doesn't work reliably, as many extension developers leave it blank (according to a little research with Stackoverflow) and implement their own creative ways to specify a version…

PHP extensions are kind of weird. It's easy to check if they're loaded at all ( get_loaded_extensions() ), and there is phpversion( $extension ) to check for their version. However, it doesn't work reliably, as many extension developers leave it blank (according to a little research with Stackoverflow) and implement their own creative ways to specify a version…

Indeed. From the composer documentation (https://getcomposer.org/doc/01-basic-usage.md#platform-packages):

ext-<name> allows you to require PHP extensions (includes core extensions). Versioning can be quite inconsistent here, so it's often a good idea to set the constraint to *

I think just supporting * for now and using extension_loaded(...) is fine for an initial implementation.

ext-<name> allows you to require PHP extensions (includes core extensions). Versioning can be quite inconsistent here, so it's often a good idea to set the constraint to *

I think just supporting * for now and using extension_loaded(...) is fine for an initial implementation.

Wouldn't it be better to query a list of extensions once and inject it into the class? This would mirror the way we currently inject the MediaWiki core version.

ext-<name> allows you to require PHP extensions (includes core extensions). Versioning can be quite inconsistent here, so it's often a good idea to set the constraint to *

I think just supporting * for now and using extension_loaded(...) is fine for an initial implementation.

Wouldn't it be better to query a list of extensions once and inject it into the class? This would mirror the way we currently inject the MediaWiki core version.

Probably, yeah. I just meant more in the way of we don't have to care about the extension's version, just a boolean of whether it's loaded or not.

Change 458940 merged by jenkins-bot:
[mediawiki/core@master] registration: Let extensions add PHP version requirements

https://gerrit.wikimedia.org/r/458940

Should we track PHP extensions in a separate task and close this one or leave it open? (I might have time to work on that in a week.)

Change 462596 had a related patch set uploaded (by MGChecker; owner: MGChecker):
[mediawiki/core@master] registration: Let extensions add PHP extension requirements

https://gerrit.wikimedia.org/r/462596

a list of "abilities": multihttp -> MultiHttpClient (e.g. VE), shell -> !Shell::isDisabled() (e.g. Score).

What would multihttp check for?

MGChecker triaged this task as Normal priority.Sep 29 2018, 2:33 PM

Change 462596 merged by jenkins-bot:
[mediawiki/core@master] registration: Let extensions add PHP extension requirements

https://gerrit.wikimedia.org/r/462596

a list of "abilities": multihttp -> MultiHttpClient (e.g. VE), shell -> !Shell::isDisabled() (e.g. Score).

What would multihttp check for?

Nothing now that T139169: Add non-parallel MultiHttpClient fallback for environments that don't have curl available is fixed :)

Remaining things:

  • Make sure all mw.o documentation is up to date
  • Add release notes for the new features
  • File a separate task for extensions that should be updated.

I have implemented ability keys (in an easily expandable way) a few days ago, however I won't be able to upload and finish this change before October 15th. I would like to include this into MW 1.32 to have a (for now) feature-complete platform key for extension registration introduced in a single change.

Afterwards, I will edit documentation on Mw.org and Release notes accordingly. Should we try to identify extensions to be updated automatically or do we have to search for them manually?

Change 467094 had a related patch set uploaded (by MGChecker; owner: MGChecker):
[mediawiki/core@master] registration: Allow to require environment abilities

https://gerrit.wikimedia.org/r/467094

@Legoktm Do you have time to take a look at this?

I was confident that this patch is merged before MW 1.32 is released, but I seemingly misjusged that. I wanted to point out that there is no reference in the release notes about the changes already merged into MW 1.32 currently, as I planned to do this after all patches are merged. We should add this.

daniel added a subscriber: daniel.Dec 17 2018, 12:59 PM

What's still missing here? The ability for extensions to require a php version, as requested by this ticket, appears to have landed, plus the ability to require php extensions. The patch that is still open seems to be for specifying platform capabilities. That seems to go well beyond the scope of this ticket. Should that really block the release? Am I missing something?

I do agree with MGChecker that the additions that were made should be mentioned in the release notes. @MGChecker, can you make a patch for that?

MGChecker added a comment.EditedDec 18 2018, 12:34 AM

What's still missing here? The ability for extensions to require a php version, as requested by this ticket, appears to have landed, plus the ability to require php extensions. The patch that is still open seems to be for specifying platform capabilities. That seems to go well beyond the scope of this ticket. Should that really block the release? Am I missing something?

I aimed to implement everything @Legoktm has proposed in T197535#4310069. This might be out of scope considering the current task title, but I considered it part of this feature and considering this, part of this task as well. Maybe it's a good idea to split this task into two.

I do agree with MGChecker that the additions that were made should be mentioned in the release notes. @MGChecker, can you make a patch for that?

I should be able to, however it depends on how much time I have left to do this. If I don't have to deliver this before Christmas, it should be fine. (Meaning I can do this before 2018-12-27 in any case, but I might have the time much sooner.) I suppose you were planning to relase MW 1.32 in the beginning of January anyway.

Sorry, I dropped the ball on this. It should've been done in 1.32, but that's my fault of running out of time in November. I don't think it should remain a release blocker and we can backport it for 1.32.1.

To avoid confusion here, I'll split this in two tasks:

  • This task is just about the things implemented already. These have to be added to the Release notes before the release. I don't want this to be forgotten.
  • T212472 tracks the work about platform abilities that hasn't been merged yet.
MGChecker raised the priority of this task from Normal to High.Dec 20 2018, 11:11 PM
MGChecker raised the priority of this task from High to Unbreak Now!.Dec 20 2018, 11:16 PM
Restricted Application added subscribers: Liuxinyu970226, TerraCodes. · View Herald TranscriptDec 20 2018, 11:16 PM
MGChecker moved this task from Backlog to Blockers on the MW-1.32-release board.Dec 20 2018, 11:18 PM

Taking a look at the remaining release blockers, I'll make a Release notes patch asap, as this is the only thing that's left here.

Change 481105 had a related patch set uploaded (by MGChecker; owner: MGChecker):
[mediawiki/core@REL1_32] Backfill release notes for platform requirements

https://gerrit.wikimedia.org/r/481105

Change 481105 merged by jenkins-bot:
[mediawiki/core@master] registration: Add release notes for platform requirements

https://gerrit.wikimedia.org/r/481105

Change 481114 had a related patch set uploaded (by Legoktm; owner: MGChecker):
[mediawiki/core@REL1_32] registration: Add release notes for platform requirements

https://gerrit.wikimedia.org/r/481114

Change 481114 merged by jenkins-bot:
[mediawiki/core@REL1_32] registration: Add release notes for platform requirements

https://gerrit.wikimedia.org/r/481114

MGChecker closed this task as Resolved.Dec 21 2018, 8:56 AM

So, this should resolve remaining release blockers.

Created T212489 to add this to extensions.

Change 467094 had a related patch set uploaded (by MGChecker; owner: MGChecker):
[mediawiki/core@master] registration: Allow to require environment abilities

https://gerrit.wikimedia.org/r/467094