Page MenuHomePhabricator

MultimediaViewer thumbnailBeforeProduceHTML hook breaks other extensions parser tests
Open, LowPublic

Description

Failing Cite parser test output

In gallery, the MultimediaViewer extensions inserts additional metadata such as: data-file-width="1941" data-file-height="220".

When having both Cite and MultimediaViewer extensions installed, the Cite parser test fails for gallery.

The test is '<references> after <gallery> (bug 6164)'

Attaching my output.


Version: unspecified
Severity: normal
See Also:
T70649: phpunit failure: ParserTests::testParserTest for gallery and files with UploadWizard and data-file-*

Attached:

Details

Reference
bz67302

Event Timeline

bzimport raised the priority of this task from to High.
bzimport set Reference to bz67302.
bzimport added a subscriber: Unknown Object (MLST).
hashar created this task.Jun 30 2014, 1:57 PM

We really need tests to pass when all wmf extensions are installed together. That is preventing us from progression toward the HHVM migrating. Raising priority to High.

Tgr added a comment.Jun 30 2014, 5:43 PM

Sounds like those tests are too rigid. Maybe they could use some sort of DOM parsing and then check only for the elements which are actually used by the Cite extension, instead of trying to match the exact HTML.

As a more fragile but simpler solution, just replace the parts between <ul>...</ul> with .*? and do a regex match.

hashar added a comment.Jul 7 2014, 3:14 PM

A bit more detail.

The MultimediaViewer extension registers ThumbnailBeforeProduceHTML , which injects additional elements to the thumb <img> element such as:

data-file-width="1941" data-file-height="220"

Cite on its own has now knowledge about that.

A hack would be to have Cite parser tests to clear up registered ThumbnailBeforeProduceHTML hooks and restore them after. I am wondering whether that sounds acceptable, should I raise the issue on wikitech?

I found out ProofReadpage inserts additional data that badly interact.

A test result with most WMF extensions installed can be found at https://integration.wikimedia.org/ci/job/mediawiki-core-extensions-integration/65/testReport/

Modifications to the parser like this will most likely never be supported in Parsoid, and they also make html output less consistent between wikis, and its imho a bad practice anyway.

I'd recommend finding another way to communicate this data or justify its general usefulness and put it in mediawiki-core.

If that's too complicated for the short term, the test could be simplified by either using a regex for the html output (with wildcards for additional attributes), as implementing a DOM tester is fair amount of extra overhead that requires research/implementation that is unlikely to be devotes to such a minor thing.

Unregistering the hook should be a last resort as it is effectively the same as not running the tests with extensions installed, which is exactly the opposite of what bug 67216 tries to achieve. Such change should leave a FIXME comment and file a bug.

  • Bug 68649 has been marked as a duplicate of this bug. ***

Lets rephrase the bug. The MultimediaViewer extension register a thumbnailBeforeProduceHTML hook which ends up breaking other extensions parser tests because the HTML is altered.

We can make parser tests to support regex and adjust the expected HTML with some wildcard regex.

Change 151833 had a related patch set uploaded by Hashar:
Support regex to match parser tests output

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

Gilles moved this task from Untriaged to Prototyping on the Multimedia board.Nov 24 2014, 4:10 PM
Spage moved this task from Prototyping to Next up on the Multimedia board.Nov 24 2014, 11:53 PM
Spage moved this task from Next up to Prototyping on the Multimedia board.
TheDJ added a subscriber: TheDJ.EditedMar 28 2015, 7:10 PM

Ehm, how about we just add:

!class_exists( 'ParserTestResult' )

around the block that adds these elements in to DOM ?
It's what I've been doing locally for a while now, because I'm so annoyed by them.

Or use the ParserTestParser Hook to flip a switch on a variable for the extension.

Gilles moved this task from Prototyping to Untriaged on the Multimedia board.Apr 6 2015, 9:22 AM
Jdforrester-WMF moved this task from Untriaged to Backlog on the Multimedia board.Sep 4 2015, 6:44 PM
Restricted Application added subscribers: Matanya, Aklapper. · View Herald TranscriptSep 4 2015, 6:44 PM

Mass-removing the Multimedia tag from MediaViewer tasks, as this is now being worked on by the Reading department, not Editing's Multimedia team.

Is this still breaking things?

Is it still a high priority?

Is it still a high priority?

Yes, unless T69216 suddenly stopped being a goal and/or MultimediaViewer stopped being something we want to test.

@Nemo_bis MultimediaViewer does get tested, just not alongside all other extensions on Jenkins, so your comment doesn't make sense to me in that respect.

I cannot comment about whether T69216 is still a goal - @hashar is this something the Release Engineering team is pushing for this quarter? MultimediaViewer is not a focus for Reading web in this quarter, but I can make sure this gets attention if T69216 is a focus.

greg added a subscriber: greg.Oct 1 2015, 3:09 AM

I cannot comment about whether T69216 is still a goal - @hashar is this something the Release Engineering team is pushing for this quarter? MultimediaViewer is not a focus for Reading web in this quarter, but I can make sure this gets attention if T69216 is a focus.

It's an unfunded goal, if that makes any sense :) IOW: it's not on our Q2 plan, per se: https://www.mediawiki.org/wiki/Wikimedia_Release_Engineering_Team/Goals/201516Q2

We still are, however, constantly striving for improvement there and along other vectors, eg: T87781 and others.

Jdlrobson lowered the priority of this task from High to Normal.Oct 1 2015, 8:55 PM

Okay bumping down to normal. We'll still have a look at it if we have bandwidth but right now we are trying to minimise work on MultimediaViewer... hopefully at the end of the quarter we'll get some time to help out here. Is this an good first bug task potentially that we could seek a volunteer to work on in the meantime?

Tgr added a comment.Oct 1 2015, 10:31 PM

I still don't think this is MediaViewer's problem. Extensions should not assume exact output in their tests. The Cite test is failing on the exact same output it will produce in the WMF cluster. What is the point of testing all WMF extensions together if not testing what output they will produce on the cluster?

In T69302#1695544, @Tgr wrote:

I still don't think this is MediaViewer's problem.

It's a problem in that it makes its testing harder. :-)

Extensions should not assume exact output in their tests.

Well, they do, and core does too. This is a general problem, see http://lists.wikimedia.org/pipermail/wikitech-l/2014-July/077481.html

T69216 is on stall since January 2015. Mainly because I am distracted with a lot of other things. But we should probably resurrect it with a limited scope such as only extensions deployed on Wikimedia.

In theory the job could fetch the list of deps from mediawiki/tools/release/make-wmf-branch and use that as a list of repo to clone and branches to fallback to.

As this task is 2 years old, and hasn't been touched since October 2015, it will be closed. Please (politely) re-open the task if it remains an issue. :)

MBinder_WMF closed this task as Declined.Aug 4 2016, 8:23 PM
Legoktm reopened this task as Open.Aug 4 2016, 8:24 PM
Legoktm added a subscriber: Legoktm.

As this task is 2 years old, and hasn't been touched since October 2015, it will be closed. Please (politely) re-open the task if it remains an issue. :)

That's not a valid reason to close tasks.

Tgr added a comment.Aug 4 2016, 8:42 PM

I think it makes sense, it catches the tasks which have long been fixed / made invalid but nobody noticed. But yeah, this is not one of them.

MarkTraceur lowered the priority of this task from Normal to Low.Apr 20 2017, 3:16 PM
MarkTraceur moved this task from Backlog to Triaged but Future on the MediaViewer board.

Coming back to MMV bugs after some significant amount of time, I have to agree with @Tgr that, while this may make it harder to test the extension alongside other extensions, it is absolutely true that the tests' assumptions are mistaken. As this is only within the MediaViewer project, I'm going to lower the priority on this once again and move it to a holding pen on our workboard. If the maintainers of the Cite extension want to take over this task and alter how their tests work, I would be happy to see an increase in priority.

Krinkle removed a subscriber: Krinkle.Jul 24 2017, 8:21 PM