New hook system not compatible with how the web installer expects LoadExtensionSchemaUpdates to work
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Legoktm
	Jul 25 2020, 1:06 AM

Description

The web installer expects that the LoadExtensionSchemaUpdates hook handler is an old-style hook that is self-contained, all it does is load that hook and run it without any other parts of the extension.

I noticed this while reviewing https://gerrit.wikimedia.org/r/c/mediawiki/extensions/OAuthRateLimiter/+/613282 in which the hook can't be called without invoking the OAuthRateLimiterTierManager service, except that won't be loaded in the web installer.

Because this system is rather fragile and I want to replace it (T258852: Replace LoadExtensionSchemaUpdates hook with static data in extension.json), I would suggest we don't allow the LoadExtensionSchemaUpdates hook to be used in the new system. Or it would need to be done in a way that doesn't have any service dependencies.

codesearch says so far only Wikibase is using it, and well, Wikibase isn't compatible with the web installer anyways.

Details

	Subject	Repo	Branch	Lines +/-
	Prevent service injection to LoadExtensionSchemaUpdates hook	mediawiki/core	REL1_35	+96 -6
	Prevent service injection to LoadExtensionSchemaUpdates hook	mediawiki/core	master	+96 -7

Customize query in gerrit

Related Objects

Mentioned In: T263510: The handler for the hook LoadExtensionSchemaUpdates registered in OAuthRateLimiter/extension.json has a service dependency, but this hook does not allow it
Mentioned Here: T258852: Replace LoadExtensionSchemaUpdates hook with static data in extension.json

Event Timeline

Legoktm created this task.Jul 25 2020, 1:06 AM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 25 2020, 1:06 AM

Legoktm updated the task description. (Show Details)Jul 25 2020, 1:28 AM

How about an option to HookContainer::run() that prevents a hook handler from having service dependencies? I'll submit a WIP patch along these lines.

Change 619166 had a related patch set uploaded (by Tim Starling; owner: Tim Starling):
[mediawiki/core@master] [WIP] Add noServices option to HookContainer::run()

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

gerritbot added a project: Patch-For-Review.Aug 10 2020, 12:25 AM

Of course, I would also support T258852. I just don't like the idea of a hook that only works with the old hooks system.

In T258851#6371577, @tstarling wrote:

How about an option to HookContainer::run() that prevents a hook handler from having service dependencies? I'll submit a WIP patch along these lines.

Sounds good to me. Do you think you could do that in time for backporting to 1.35? I'll implement the web installer part then.

In T258851#6371721, @tstarling wrote:

Of course, I would also support T258852. I just don't like the idea of a hook that only works with the old hooks system.

Fair enough. T258852 is my long-term plan but I suspect we're going to need the hook anyways for quite a while as migration happens.

In T258851#6375016, @Legoktm wrote:

In T258851#6371577, @tstarling wrote:

How about an option to HookContainer::run() that prevents a hook handler from having service dependencies? I'll submit a WIP patch along these lines.

Sounds good to me. Do you think you could do that in time for backporting to 1.35? I'll implement the web installer part then.

OK, the change is now ready for review. I updated the change to include using noServices for LoadExtensionSchemaUpdates, since that was only one extra line of code, and it's nice to have an example.

DannyS712 subscribed.Aug 12 2020, 10:47 AM

Change 619166 merged by jenkins-bot:
[mediawiki/core@master] Prevent service injection to LoadExtensionSchemaUpdates hook

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

Maintenance_bot removed a project: Patch-For-Review.Sep 21 2020, 8:10 PM

ReleaseTaggerBot added a project: MW-1.36-notes (1.36.0-wmf.10; 2020-09-22).Sep 21 2020, 9:01 PM

Reedy mentioned this in T263510: The handler for the hook LoadExtensionSchemaUpdates registered in OAuthRateLimiter/extension.json has a service dependency, but this hook does not allow it.Sep 22 2020, 12:56 AM

Change 658618 had a related patch set uploaded (by Reedy; owner: Tim Starling):
[mediawiki/core@REL1_35] Prevent service injection to LoadExtensionSchemaUpdates hook

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

gerritbot added a project: Patch-For-Review.Jan 27 2021, 1:22 AM

Reedy closed this task as Resolved.Jan 27 2021, 1:27 AM

Reedy assigned this task to tstarling.

Change 658618 merged by jenkins-bot:
[mediawiki/core@REL1_35] Prevent service injection to LoadExtensionSchemaUpdates hook

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

ReleaseTaggerBot added a project: MW-1.35-notes.Jan 27 2021, 2:00 AM

Maintenance_bot removed a project: Patch-For-Review.Jan 27 2021, 2:10 AM