Page MenuHomePhabricator
Create Task
Maniphest T255704

Fatal LogicException: Role mediainfo is already defined (DiscussionTools conflicts with WikibaseMediaInfo)
Closed, ResolvedPublicPRODUCTION ERROR
Actions

Assigned To
matmarex
Authored By
thcipriani
Jun 17 2020, 6:39 PM
Tags
Referenced Files
None
Subscribers
Addshore
Agusbou2015
Aklapper
daniel
• Esanders
Jdforrester-WMF
Jdlrobson
View All 15 Subscribers

Description

Error message
LogicException: Role mediainfo is already defined
Stack Trace
#0 /srv/mediawiki/php-1.35.0-wmf.37/includes/Revision/SlotRoleRegistry.php(115): MediaWiki\Revision\SlotRoleRegistry->defineRole(string, Closure)
#1 /srv/mediawiki/php-1.35.0-wmf.37/extensions/WikibaseMediaInfo/src/WikibaseMediaInfoHooks.php(68): MediaWiki\Revision\SlotRoleRegistry->defineRoleWithModel(string, string)
#2 [internal function]: Wikibase\MediaInfo\WikibaseMediaInfoHooks::Wikibase\MediaInfo\{closure}(MediaWiki\Revision\SlotRoleRegistry, MediaWiki\MediaWikiServices)
#3 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(458): call_user_func_array(Closure, array)
#4 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(419): Wikimedia\Services\ServiceContainer->createService(string)
#5 /srv/mediawiki/php-1.35.0-wmf.37/includes/MediaWikiServices.php(1184): Wikimedia\Services\ServiceContainer->getService(string)
#6 /srv/mediawiki/php-1.35.0-wmf.37/includes/ServiceWiring.php(1031): MediaWiki\MediaWikiServices->getSlotRoleRegistry()
#7 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(451): Wikimedia\Services\ServiceContainer->{closure}(MediaWiki\MediaWikiServices)
#8 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(419): Wikimedia\Services\ServiceContainer->createService(string)
#9 /srv/mediawiki/php-1.35.0-wmf.37/includes/MediaWikiServices.php(1119): Wikimedia\Services\ServiceContainer->getService(string)
#10 /srv/mediawiki/php-1.35.0-wmf.37/includes/ServiceWiring.php(1013): MediaWiki\MediaWikiServices->getRevisionStoreFactory()
#11 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(451): Wikimedia\Services\ServiceContainer->{closure}(MediaWiki\MediaWikiServices)
#12 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(419): Wikimedia\Services\ServiceContainer->createService(string)
#13 /srv/mediawiki/php-1.35.0-wmf.37/includes/MediaWikiServices.php(1111): Wikimedia\Services\ServiceContainer->getService(string)
#14 /srv/mediawiki/php-1.35.0-wmf.37/includes/ServiceWiring.php(999): MediaWiki\MediaWikiServices->getRevisionStore()
#15 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(451): Wikimedia\Services\ServiceContainer->{closure}(MediaWiki\MediaWikiServices)
#16 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(419): Wikimedia\Services\ServiceContainer->createService(string)
#17 /srv/mediawiki/php-1.35.0-wmf.37/includes/MediaWikiServices.php(1095): Wikimedia\Services\ServiceContainer->getService(string)
#18 /srv/mediawiki/php-1.35.0-wmf.37/includes/ServiceWiring.php(885): MediaWiki\MediaWikiServices->getRevisionLookup()
#19 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(451): Wikimedia\Services\ServiceContainer->{closure}(MediaWiki\MediaWikiServices)
#20 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(419): Wikimedia\Services\ServiceContainer->createService(string)
#21 /srv/mediawiki/php-1.35.0-wmf.37/includes/MediaWikiServices.php(1039): Wikimedia\Services\ServiceContainer->getService(string)
#22 /srv/mediawiki/php-1.35.0-wmf.37/includes/ServiceWiring.php(130): MediaWiki\MediaWikiServices->getPermissionManager()
#23 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(451): Wikimedia\Services\ServiceContainer->{closure}(MediaWiki\MediaWikiServices)
#24 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(419): Wikimedia\Services\ServiceContainer->createService(string)
#25 /srv/mediawiki/php-1.35.0-wmf.37/includes/MediaWikiServices.php(493): Wikimedia\Services\ServiceContainer->getService(string)
#26 /srv/mediawiki/php-1.35.0-wmf.37/extensions/GlobalPreferences/includes/Hooks.php(198): MediaWiki\MediaWikiServices->getAuthManager()
#27 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(451): GlobalPreferences\Hooks::GlobalPreferences\{closure}(MediaWiki\MediaWikiServices)
#28 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(419): Wikimedia\Services\ServiceContainer->createService(string)
#29 /srv/mediawiki/php-1.35.0-wmf.37/includes/MediaWikiServices.php(1047): Wikimedia\Services\ServiceContainer->getService(string)
#30 /srv/mediawiki/php-1.35.0-wmf.37/extensions/GlobalPreferences/includes/Hooks.php(294): MediaWiki\MediaWikiServices->getPreferencesFactory()
#31 /srv/mediawiki/php-1.35.0-wmf.37/extensions/GlobalPreferences/includes/Hooks.php(43): GlobalPreferences\Hooks::getPreferencesFactory()
#32 /srv/mediawiki/php-1.35.0-wmf.37/includes/HookContainer/HookContainer.php(319): GlobalPreferences\Hooks::onUserLoadOptions(User, array)
#33 /srv/mediawiki/php-1.35.0-wmf.37/includes/HookContainer/HookContainer.php(131): MediaWiki\HookContainer\HookContainer->callLegacyHook(string, array, array, array)
#34 /srv/mediawiki/php-1.35.0-wmf.37/includes/HookContainer/HookRunner.php(4358): MediaWiki\HookContainer\HookContainer->run(string, array)
#35 /srv/mediawiki/php-1.35.0-wmf.37/includes/user/UserOptionsManager.php(557): MediaWiki\HookContainer\HookRunner->onUserLoadOptions(User, array)
#36 /srv/mediawiki/php-1.35.0-wmf.37/includes/user/UserOptionsManager.php(135): MediaWiki\User\UserOptionsManager->loadUserOptions(User, integer)
#37 /srv/mediawiki/php-1.35.0-wmf.37/includes/user/UserOptionsLookup.php(123): MediaWiki\User\UserOptionsManager->getOption(User, string, integer, boolean, integer)
#38 /srv/mediawiki/php-1.35.0-wmf.37/includes/user/User.php(2740): MediaWiki\User\UserOptionsLookup->getIntOption(User, string, integer)
#39 /srv/mediawiki/php-1.35.0-wmf.37/includes/user/User.php(2930): User->getIntOption(string)
#40 /srv/mediawiki/php-1.35.0-wmf.37/includes/linker/LinkRendererFactory.php(99): User->getStubThreshold()
#41 /srv/mediawiki/php-1.35.0-wmf.37/includes/ServiceWiring.php(521): MediaWiki\Linker\LinkRendererFactory->createForUser(User)
#42 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(451): Wikimedia\Services\ServiceContainer->{closure}(MediaWiki\MediaWikiServices)
#43 /srv/mediawiki/php-1.35.0-wmf.37/includes/libs/services/ServiceContainer.php(419): Wikimedia\Services\ServiceContainer->createService(string)
#44 /srv/mediawiki/php-1.35.0-wmf.37/includes/MediaWikiServices.php(825): Wikimedia\Services\ServiceContainer->getService(string)
#45 /srv/mediawiki/php-1.35.0-wmf.37/includes/page/Article.php(165): MediaWiki\MediaWikiServices->getLinkRenderer()
#46 /srv/mediawiki/php-1.35.0-wmf.37/includes/page/Article.php(212): Article->__construct(Title)
#47 /srv/mediawiki/php-1.35.0-wmf.37/includes/page/Article.php(228): Article::newFromTitle(Title, RequestContext)
#48 /srv/mediawiki/php-1.35.0-wmf.37/includes/actions/Action.php(186): Article::newFromWikiPage(WikiPage, RequestContext)
#49 /srv/mediawiki/php-1.35.0-wmf.37/includes/MediaWiki.php(166): Action::getActionName(RequestContext)
#50 /srv/mediawiki/php-1.35.0-wmf.37/includes/MediaWiki.php(903): MediaWiki->getAction()
#51 /srv/mediawiki/php-1.35.0-wmf.37/includes/MediaWiki.php(543): MediaWiki->main()
#52 /srv/mediawiki/php-1.35.0-wmf.37/index.php(47): MediaWiki->run()
#53 /srv/mediawiki/w/index.php(3): require(string)
#54 {main}
Impact

…

Notes
  • Spike on commonswiki for render api events affecting all kinds of pages including wikitext and JS pages.
  • 8,000 exceptions all at once, then nothing...

Details

SubjectRepoBranchLines +/-
Use $wgLocaltimezone global instead of request contextmediawiki/extensions/DiscussionToolswmf/1.35.0-wmf.36+3 -2
Revert "Disable DiscussionTools at beta commonswiki"operations/mediawiki-configmaster+0 -1
Disable DiscussionTools at beta commonswikioperations/mediawiki-configmaster+1 -0
Use $wgLocaltimezone global instead of request contextmediawiki/extensions/DiscussionToolsmaster+3 -2
Use $wgLocaltimezone global instead of request contextmediawiki/extensions/DiscussionToolswmf/1.35.0-wmf.37+3 -2
Customize query in gerrit

Related Objects

Event Timeline

thcipriani created this task.Jun 17 2020, 6:39 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 17 2020, 6:39 PM
brennen added a project: User-brennen.Jun 17 2020, 6:44 PM
brennen moved this task from Backlog to Logs/Train on the User-brennen board.
Krinkle renamed this task from Role mediainfo is already defined to Fatal LogicException: Role mediainfo is already defined.Jun 17 2020, 6:45 PM
Krinkle updated the task description. (Show Details)
Krinkle edited projects, added WikibaseMediaInfo, Structured Data Engineering; removed Wikidata, Wikidata-Campsite.
Restricted Application added a project: Structured-Data-Backlog. · View Herald TranscriptJun 17 2020, 6:45 PM
Krinkle edited Stack Trace. (Show Details)Jun 17 2020, 6:45 PM
brennen mentioned this in T254174: 1.35.0-wmf.37 deployment blockers.Jun 17 2020, 6:45 PM
Krinkle triaged this task as High priority.Jun 17 2020, 6:45 PM
• Mholloway subscribed.Jun 17 2020, 6:49 PM
• Urbanecm added subscribers: matmarex, • Urbanecm.Jun 17 2020, 6:54 PM

This is relevant to https://sal.toolforge.org/log/SrGDw3IBLkHzneNN3dWT (reverted in https://sal.toolforge.org/log/ybGGw3IBLkHzneNNQNbu), in which I tried to deploy https://gerrit.wikimedia.org/r/c/operations/mediawiki-config/+/599307 at @matmarex's request. I don't think there's anything to look into (except why DiscussionTools dislikes MCR :)).

Addshore subscribed.Jun 17 2020, 7:02 PM
• taavi subscribed.Jun 17 2020, 7:02 PM
matmarex renamed this task from Fatal LogicException: Role mediainfo is already defined to Fatal LogicException: Role mediainfo is already defined (DiscussionTools conflicts with WikibaseMediaInfo).Jun 17 2020, 7:07 PM
matmarex merged a task: T255708: DiscussionTools conflicts with WikibaseMediaInfo.
matmarex added a project: DiscussionTools.
Krinkle merged a task: T255705: [XupjZqwQBHcAAAaR1i8AAAAR] / LogicException from line 88 of /srv/mediawiki/php-master/includes/Revision/SlotRoleRegistry.php.Jun 17 2020, 7:09 PM
Krinkle added subscribers: Jdlrobson, RhinosF1, Jdforrester-WMF and 2 others.
gerritbot added a comment.Jun 17 2020, 7:09 PM

Change 606244 had a related patch set uploaded (by Krinkle; owner: Urbanecm):
[operations/mediawiki-config@master] Disable DiscussionTools at beta

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

gerritbot added a project: Patch-For-Review.Jun 17 2020, 7:10 PM
matmarex added a comment.Jun 17 2020, 7:10 PM

Relevant code in WikibaseMediaInfo:

	public static function onMediaWikiServices( MediaWikiServices $services ) {
		$services->addServiceManipulator( 'SlotRoleRegistry', function ( SlotRoleRegistry $registry ) {
			$registry->defineRoleWithModel(
				/* role */ 'mediainfo',
				/* content handler */ MediaInfoContent::CONTENT_MODEL_ID
				/*, layout – we want to set "prepend" in future, once MediaWiki supports that */
			);
		} );
	}

There is nothing else that tried to define the 'mediainfo' role. So as far as I can see, the only way for this to occur is for a hook handler for onMediaWikiServices to call the hook itself again (probably indirectly via 20 layers of services).

Krinkle subscribed.Jun 17 2020, 7:14 PM
In T255704#6232958, @Urbanecm wrote:

This is relevant to https://sal.toolforge.org/log/SrGDw3IBLkHzneNN3dWT (reverted in https://sal.toolforge.org/log/ybGGw3IBLkHzneNNQNbu), in which I tried to deploy https://gerrit.wikimedia.org/r/c/operations/mediawiki-config/+/599307 at @matmarex's request. I don't think there's anything to look into (except why DiscussionTools dislikes MCR :)).

For the record, the revert is: https://gerrit.wikimedia.org/r/#/c/operations/mediawiki-config/+/606235/

gerritbot added a comment.Jun 17 2020, 7:17 PM

Change 606244 merged by jenkins-bot:
[operations/mediawiki-config@master] Disable DiscussionTools at beta commonswiki

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

• Mholloway mentioned this in T255157: [Bug] Recommendation API returning error 500 in beta cluster blocks restbase CI.Jun 17 2020, 7:25 PM
matmarex claimed this task.Jun 17 2020, 7:26 PM
matmarex mentioned this in T253943: Enable DiscussionTools at all wikis via query string.
ppelberg added a project: Editing-team (Q3 2019-2020 Kanban Board).Jun 17 2020, 8:03 PM
ppelberg moved this task from Needs Discussion / Investigation to Doing on the Editing-team (Q3 2019-2020 Kanban Board) board.
Maintenance_bot removed a project: Patch-For-Review.Jun 17 2020, 8:10 PM
• Esanders subscribed.Jun 17 2020, 8:44 PM

It seems the DiscussionTools onRegistration hook (which runs much earlier that other hooks) is triggering this problem. It accesses the core config to check if Localtimezone is set, and accessing the core config that early is confusing WikibaseMediaInfo.

Unless we are explicitly not allowed to access the config during onRegistration the problem is still with WikibaseMediaInfo, but we can at least work around it in DiscussionTools by using the $wg global instead.

gerritbot added a comment.Jun 17 2020, 8:49 PM

Change 606264 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/extensions/DiscussionTools@master] Use $wgLocaltimezone global instead of request context

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

gerritbot added a project: Patch-For-Review.Jun 17 2020, 8:49 PM
matmarex added a comment.EditedJun 17 2020, 8:59 PM

I think this is a bug in MediaWikiServices. When resetGlobalInstance() is called, it runs the 'MediaWikiServices' hook (which adds the manipulator from WikibaseMediaInfo), and then calls importWiring() (which copies all of the existing manipulators – including the one from WikibaseMediaInfo). This is how we end up with two copies of it, and when the second copy is called, it throws this exception.

I don't know why DiscussionTools triggers this, but I don't think it's the cause of the problem. I guess @Esanders explained it, I didn't see his comment before posting mine.

I'm not sure how this should be fixed in MediaWikiServices. It worries me a bit that a method which is supposed to reset the services actually doesn't.

daniel added projects: Platform Engineering, User-Daniel, Dependency injection.Jun 17 2020, 9:21 PM

Tagging CPT to have a look, in the light of @matmarex analysis.

I'm not sure how this should be fixed in MediaWikiServices. It worries me a bit that a method which is supposed to reset the services actually doesn't.

Besides fixing a bug, there are two bigger issues to look at here:

  1. Ideally there should be no need to reset services. We should revisit the reasons (basically, because some services are needed before extensions have initialized, and others are impacted by extensions having been initialized)
  2. We should revisit the idea of ServiceManipulators. It was introduced to give extensions a way to inject SlotRoleHandlers. Perhaps that should be changed to use ObjectFactory and ExtensionRegistry, like we do for the new hook system. I think that would be much cleaner.
gerritbot added a comment.Jun 17 2020, 9:34 PM

Change 606264 merged by jenkins-bot:
[mediawiki/extensions/DiscussionTools@master] Use $wgLocaltimezone global instead of request context

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

gerritbot added a comment.Jun 17 2020, 9:58 PM

Change 606279 had a related patch set uploaded (by Bartosz Dziewoński; owner: Esanders):
[mediawiki/extensions/DiscussionTools@wmf/1.35.0-wmf.37] Use $wgLocaltimezone global instead of request context

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

ReleaseTaggerBot added a project: MW-1.35-notes (1.35.0-wmf.38; 2020-06-23).Jun 17 2020, 10:00 PM
Agusbou2015 subscribed.Jun 17 2020, 10:27 PM
gerritbot added a comment.Jun 17 2020, 11:05 PM

Change 606279 merged by jenkins-bot:
[mediawiki/extensions/DiscussionTools@wmf/1.35.0-wmf.37] Use $wgLocaltimezone global instead of request context

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

gerritbot added a comment.Jun 17 2020, 11:05 PM

Change 606292 had a related patch set uploaded (by Urbanecm; owner: Esanders):
[mediawiki/extensions/DiscussionTools@wmf/1.35.0-wmf.36] Use $wgLocaltimezone global instead of request context

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

gerritbot added a comment.Jun 17 2020, 11:07 PM

Change 606293 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[operations/mediawiki-config@master] Revert "Disable DiscussionTools at beta commonswiki"

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

gerritbot added a comment.Jun 17 2020, 11:08 PM

Change 606293 merged by jenkins-bot:
[operations/mediawiki-config@master] Revert "Disable DiscussionTools at beta commonswiki"

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

gerritbot added a comment.Jun 17 2020, 11:13 PM

Change 606292 merged by Urbanecm:
[mediawiki/extensions/DiscussionTools@wmf/1.35.0-wmf.36] Use $wgLocaltimezone global instead of request context

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

Stashbot mentioned this in T252264: Deploy Reply tool via query string parameter to zh.wiki.Jun 17 2020, 11:21 PM

Mentioned in SAL (#wikimedia-operations) [2020-06-17T23:21:49Z] <urbanecm@deploy1001> Synchronized php-1.35.0-wmf.37/extensions/DiscussionTools/includes/Hooks.php: 4551d29: Use $wgLocaltimezone global instead of request context (T252264; T253943; T255704) (duration: 00m 58s)

Stashbot added a comment.Jun 17 2020, 11:23 PM

Mentioned in SAL (#wikimedia-operations) [2020-06-17T23:23:20Z] <urbanecm@deploy1001> Synchronized php-1.35.0-wmf.36/extensions/DiscussionTools/includes/Hooks.php: ff01083: Use $wgLocaltimezone global instead of request context (T255704) (duration: 00m 57s)

matmarex moved this task from Doing to Blocked / Needs More Work on the Editing-team (Q3 2019-2020 Kanban Board) board.Jun 17 2020, 11:52 PM

The workaround fixes the exception, so from Editing's perspective I think this is done, but I'll leave it open for CPT to have a look at the underlying problem, per @daniel.

ReleaseTaggerBot edited projects, added MW-1.35-notes (1.35.0-wmf.36; 2020-06-09); removed MW-1.35-notes (1.35.0-wmf.38; 2020-06-23).Jun 18 2020, 12:00 AM
Maintenance_bot removed a project: Patch-For-Review.Jun 18 2020, 12:10 AM
• Ramsey-WMF moved this task from Triage to Tracking on the Structured-Data-Backlog board.Jun 18 2020, 4:08 PM
Krinkle closed this task as Resolved.Jun 23 2020, 3:02 PM

The onRegistration callback is imho for basic prepping of globals or constants for things that extension.json does (not) yet support.

The purpose for which it is used here by DT is imho inappropiocate and is part of a general pattern we've been moving away from in core. Pre-processing logic or validation of config should happen lazily from wherever something needs it. E.g. when interacting with a part of the system where DT is not involved, this logic does not need to happen, for example. If you find multiple parts of the code base and/or other extension need to access the configuration, use a public method as the stable interface to obtain and validate it as needed.

For the case of wgLocaltimezone checking, I suppose it depends on what part of DT is actually impacted by it. For example if there is a main service class where most of the server-side logic resides, it could be done by its constructor. Or if it (also) relates to client code, one could use any number of hooks (so long as it is post-setup), e.g. ResourceLoaderRegisterModules if need be.

matmarex added a comment.Jun 29 2020, 3:30 PM

@Krinkle I don't agree with that. Our check for $wgLocaltimezone seems similar to e.g. checking whether another extension is installed or checking for minimal MediaWiki version, both of which happen during extension registration.

Krinkle added a comment.Jul 6 2020, 7:02 PM
In T255704#6264586, @matmarex wrote:

@Krinkle I don't agree with that. Our check for $wgLocaltimezone seems similar to e.g. checking whether another extension is installed or checking for minimal MediaWiki version, both of which happen during extension registration.

During the extension registry phase, information about the environment (web server, OS, PHP) and MW core (MW version) are fixed.

Information about the site configuratation is not fixed and unsafe there because 1) Most of Setup.php hasn't happened yet, so various compat or aliases won't have expanded yet, 2) Other extension registration callbacks haven't run yet, which might declare their own magic defaults or compat aliases, and 3) $wgExtensionFunctions hasn't run yet.

What you are describing is an unconditional callback that can safely observe the real site config and then decide to abort. This is not currently supported and does not exist in MediaWiki today. This logic should imho be done in a service class where it is also testable and naturally deferred to relevant contexts only.

matmarex mentioned this in T267146: LogicException: Role mediainfo is already defined.Nov 4 2020, 2:21 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits