Page MenuHomePhabricator

wikibase.entityPage.entityLoaded does not fire for entities with no data yet entered
Closed, ResolvedPublicBUG REPORT

Description

This ticket identified a bug that made it impossible to edit captions for many files on Beta Commons. The problem was fixed for files with existing structured data as of this patch and for those without any existing data with this patch.

Ideally, the WikibaseMediaInfo extension could rely on the wikibase.entityPage.entityLoaded hook to for initialization. However, this hook currently fires only if structured data already exists for a given file. Newly-created files lack this data, and therefore the captions panel UI was broken on these pages prior to the recent workaround.

It seems like there are two possible solutions here: either 1) ensure that the wikibase.entityPage.entityLoaded always fires (a Wikibase change), or 2) changing the way that CaptionsPanel gets initialized (a MediaInfo change).

If we go down the latter route, some refactoring may be necessary in the CaptionsPanel class to allow for a widget to be created with no data and then (optionally) have that data provided at a later time based on whether or not the relevant hook gets called. This class is one of the most complex bits of JS in the extension so it may be a non-trivial change.

Let's use this ticket to discuss how to move forward.

Details

Related Gerrit Patches:

Event Timeline

egardner created this task.Jul 29 2019, 9:05 PM
Restricted Application added a project: Multimedia. · View Herald TranscriptJul 29 2019, 9:05 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
egardner triaged this task as High priority.Jul 29 2019, 9:06 PM
egardner updated the task description. (Show Details)
Ramsey-WMF added subscribers: Cparle, Ramsey-WMF.

This is pretty much top priority for us now. 🔴 Let's address as soon as we can. Thanks!

Ramsey-WMF moved this task from Untriaged to Next up on the Multimedia board.Jul 29 2019, 9:28 PM

Change 526181 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/WikibaseMediaInfo@master] Add wikibase.entityPage.entityLoaded as dependency of the wikibase.mediainfo.filePageDisplay

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

It's hard for me to test mediainfo things but given that new uploads still have mediainfo entity (I tried mw.config.values.wbEntityId in the given page and it gave me "M65815") then it means you just need to make the module wikibase.entityPage.entityLoaded dependency of your bootstrap module. I brought back one of my patches to fix it. Can you take a look and test it? Thanks

Mholloway added a comment.EditedJul 29 2019, 9:32 PM

What's happening in the case of a newly created file is that wikibase.entityPage.entityLoaded.js hits Special:EntityData/Mnnnn.json, gets a 404 back, and the getJSON callback never gets called.

I think we could avoid a larger overhaul just by adding an error handler to the getJSON call in wikibase.entityPage.entityLoaded.js to handle this expected result. This fixes the problem, for example:

	$.getJSON( url.toString(), function ( data ) {
            /* existing handling code */
	} ).error( function( err ) {
            if ( err.status === 404 ) {
	        mw.hook( 'wikibase.entityPage.entityLoaded' ).fire( {} );
	    }
	} );

Alternatively, if there's some reason WMDE doesn't want that to happen, maybe we could define a new hook to fire in the 404 case ( wikibase.entityPage.entityLoadedEmpty or something ).

Of course, this doesn't preclude a refactor of CaptionsPanel.js if that would be beneficial.

@Ladsgroup I have a dev environment with MediaInfo set up. Pulled down your patch but I still see the same bug: a newly-created file can't initialize the captions panel correctly because that hook isn't getting fired.

I think that @Mholloway's suggestion could be a good short-term solution if that's acceptable. Since this is high-priority / unbreak-now, it would be great to find a fix that doesn't require a significant refactor. There are still things we probably do need to change in how the CaptionsPanel gets set up, but ideally that would not happen in a rush to avoid derailing a release.

alaa_wmde added a subscriber: alaa_wmde.EditedJul 29 2019, 11:01 PM

... 1) ensure that the wikibase.entityPage.entityLoaded always fires (a Wikibase change), or ...

I think we could avoid a larger overhaul just by adding an error handler to the getJSON call in wikibase.entityPage.entityLoaded.js to handle this expected result.

Practically, that is a breaking change, and might require an announcement and many other dependent users to update their code so that they start checking for the passed value into the handler.

Also practically speaking, that hook doesn't seem to be used in many places (no clue about tools, gadgets etc) according to https://codesearch.wmflabs.org/search/?q=wikibase.entityPage.entityLoaded&i=nope&files=&repos= (how much can we trust codesearch like that actually?)

egardner added a comment.EditedJul 29 2019, 11:02 PM

Did some digging into wikibase.entityPage.entityLoaded.js in Wikibase.

It looks like the $.getJSON call is returning an error for my newly-created file page, but that error is not a 404 error from the server. It is a parsererror from JS.

Here's what I'm doing to log:

$.getJSON( url.toString(), function ( data ) {
	var wbEntity;

	if ( !data || !data.entities || !data.entities[ entityId ] ) {
		return;
	}

	wbEntity = data.entities[ entityId ];

	if ( wbEntity ) {
		// Note this assumes "wbEntity" contains valid JSON, and will throw
		// an error otherwise.
		mw.hook( 'wikibase.entityPage.entityLoaded' ).fire( deepFreeze( wbEntity ) );
	}
} ).fail( function ( jqxhr, textStatus, error ) {
	console.log( textStatus );
	console.log( error );
} );

error here is: SyntaxError: "JSON.parse: unexpected character at line 1 column 1 of the JSON data". I suspect this is happening because JSON.parse is being called with no actual data to work with. Calling JSON.parse("") throws the same error.

Finally, the error ResponseText includes this information:

Warning: Wikibase\\Lib\\Store\\Sql\\WikiPageEntityRevisionLookup::getEntityRevision: Entity not loaded for M58 [Called from Wikibase\\Lib\\Store\\Sql\\WikiPageEntityRevisionLookup::getEntityRevision in /var/www/mediawiki/extensions/Wikibase/lib/includes/Store/Sql/WikiPageEntityRevisionLookup.php at line 148] in /var/www/mediawiki/includes/debug/MWDebug.php on line <i>309</I>

Adding mw.hook( 'wikibase.entityPage.entityLoaded' ).fire( {} ); inside the fail handler for $.getJSON does restore the correct CaptionsPanel behavior on a new file (the "Edit" controls work again). But I'd like to understand the meaning of this error better before proposing this as a fix.

Change 526181 merged by jenkins-bot:
[mediawiki/extensions/WikibaseMediaInfo@master] Add wikibase.entityPage.entityLoaded as dependency of the wikibase.mediainfo.filePageDisplay

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

@egardner Hmm. Even with your code, the error text value on my machine is Not Found.

jqxhr.responseText looks similar, though:

"<br />\n<b>Warning</b>:  Wikibase\\Lib\\Store\\Sql\\WikiPageEntityRevisionLookup::getEntityRevision: Entity not loaded for M151 [Called from Wikibase\\Lib\\Store\\Sql\\WikiPageEntityRevisionLookup::getEntityRevision in /vagrant/mediawiki/extensions/Wikibase/lib/includes/Store/Sql/WikiPageEntityRevisionLookup.php at line 148] in <b>/vagrant/mediawiki/includes/debug/MWDebug.php</b> on line <b>309</b><br />\n (10.0.3.31)<!DOCTYPE html>\n<html><head><title>Not Found</title></head>\n<body><h1>Not Found</h1><p>Can&#039;t show revision 310 of entity M151.</p></body></html>\n"

And jqxhr.status is 404.

I think the code path here is that WikiPageEntityRevisionLookup is throwing a BadRevisionException since it has a revision ID but its query returns no associated entity data; EntityDataRequestHandler then catches that BadRevisionException and throws the 404. Here's the full log output associated with the error: P8821

That said, it looks like getJSON is just shorthand for making an AJAX request and parsing the result data as JSON, so it makes sense that attempting to parse an empty data result would raise a ParseError. Might just be a subtle difference in our environments.

@alaa_wmde I'm guessing that in Wikidata it's not expected behavior for an entity to have an assigned ID but no structured content, as appears to be the case in WikibaseMediaInfo before any structured data is entered, and so entityLoaded would never fail to fire for a valid entity under normal conditions. Is that correct? What do you think is the sanest thing to do in a scenario in which structured data might not yet exist?

I presume it was a deliberate choice in WikibaseMediaInfo not to initialize the mediainfo content blob until the first piece of structured data is entered, and my guess is that we don't want that to change, although I'd be interested in hearing more about it from someone who was there at the time.

Is it really plausible that anyone is relying on wikibase.entityPage.entityLoaded not firing in this scenario? Seems unlikely.

Either way, it makes sense that changing that behavior will take more discussion, possibly announcements, etc. As a workaround in the meantime I'd recommend CaptionsPanel just fetching the info directly rather than relying on the hook.

I'm guessing that in Wikidata it's not expected behavior for an entity to have an assigned ID but no structured content, as appears to be the case in WikibaseMediaInfo before any structured data is entered, and so entityLoaded would never fail to fire for a valid entity under normal conditions. Is that correct?

Yes.

I presume it was a deliberate choice in WikibaseMediaInfo not to initialize the mediainfo content blob until the first piece of structured data is entered

That's correct.

This comment was removed by alaa_wmde.

FYI had an idea for a fix in MediaInfo itself, just checking if it'll work ...

FYI had an idea for a fix in MediaInfo itself, just checking if it'll work ...

Thanks. Keep us posted if that works.

Change 526399 had a related patch set uploaded (by Cparle; owner: Cparle):
[mediawiki/extensions/WikibaseMediaInfo@master] Initialize CaptionsPanel before entityLoaded fires

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

Ok patch is passing CI ... it re-initializes the captions data after the hook fires. Not ideal, but it seems to work

Decoupling this from the train blocker since that'll be resolved as soon as the patch lands. I'll leave this task open for discussion about what to do long-term.

Mholloway renamed this task from Captions edit controls broken for newly-created files to wikibase.entityPage.entityLoaded does not fire for entities with no data yet entered.Jul 30 2019, 2:14 PM
Mholloway updated the task description. (Show Details)
Mholloway raised the priority of this task from High to Unbreak Now!.Jul 30 2019, 2:48 PM

Per discussion in -releng, raising this to UBN and a train blocker as well just so it isn't missed.

Restricted Application added a subscriber: Liuxinyu970226. · View Herald TranscriptJul 30 2019, 2:48 PM

Change 526399 merged by jenkins-bot:
[mediawiki/extensions/WikibaseMediaInfo@master] Initialize CaptionsPanel before entityLoaded fires

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

Mholloway lowered the priority of this task from Unbreak Now! to High.Jul 30 2019, 3:59 PM

Train blocker is resolved. Leaving open for discussion per my comment above.

Mholloway updated the task description. (Show Details)Jul 30 2019, 4:07 PM
Mholloway updated the task description. (Show Details)

Ok patch is passing CI ... it re-initializes the captions data after the hook fires. Not ideal, but it seems to work

That's great., and impressive to pull off that solution so quickly.

To my mind, that's the right direction. It is a common practice for such components to be reactive in that way, with the initial render probably being done prior to any data being yet retrieved from the server or more ideally be even rendered server-side, if that's what you meant by not being ideal.

Ramsey-WMF closed this task as Resolved.Aug 13 2019, 4:41 PM
Ramsey-WMF claimed this task.