DiffFormatter.php
Closed, ResolvedPublic
Actions

Description

I ran into this on Vagrant with the Echo role enabled. Don't see this error in production so it might be specific to Vagrant somehow, although it seems unlikely.

Notice: Undefined property: Diff::$edits in /vagrant/mediawiki/includes/diff/DiffFormatter.php on line 74
#0  {main}( ) .../api.php:0
#1  require( '/vagrant/mediawiki/api.php' ) .../api.php:5
#2  MediaWiki->doPostOutputShutdown( ) .../api.php:124
#3  MediaWiki->{closure:/vagrant/mediawiki/includes/MediaWiki.php:764-771}( ) .../MediaWiki.php:789
#4  MediaWiki->restInPeace( ) .../MediaWiki.php:766
#5  DeferredUpdates::doUpdates( ) .../MediaWiki.php:942
#6  DeferredUpdates::handleUpdateQueue( ) .../DeferredUpdates.php:149
#7  DeferredUpdates::run( ) .../DeferredUpdates.php:226
#8  DeferredUpdates::attemptUpdate( ) .../DeferredUpdates.php:281
#9  MWCallableUpdate->doUpdate( ) .../DeferredUpdates.php:383
#10 EchoHooks::{closure:/vagrant/mediawiki/extensions/Echo/includes/EchoHooks.php:535-542}( ) .../MWCallableUpdate.php:38 
#11 EchoDiscussionParser::generateEventsForRevision( ) .../EchoHooks.php:541
#12 EchoDiscussionParser::getChangeInterpretationForRevision( ) .../DiscussionParser.php:40
#13 EchoDiscussionParser::getMachineReadableDiff( ) .../DiscussionParser.php:480
#14 EchoDiffParser->getChangeSet( ) .../DiscussionParser.php:925
#15 UnifiedDiffFormatter->format( ) .../EchoDiffParser.php:89

The relevant code is

$diffs = new Diff( explode( "\n", $left ), explode( "\n", $right ) );
$format = new UnifiedDiffFormatter();
$diff = $format->format( $diffs );

and Diff::$edits is a public property so I don't understand what's going on here.

Details

Related Gerrit Patches:

mediawiki/services/parsoid : master

Remove phpspec

Related Objects

Mentioned Here: T166010: The Great Namespaceization and Reorg

Event Timeline

Tgr created this task.Sep 27 2019, 1:08 PM

Restricted Application added a project: Growth-Team. · View Herald TranscriptSep 27 2019, 1:08 PM

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Ok, apparently this is a namespace conflict: MediaWiki and phpspec (used by Parsoid) both have an un-namespaced Diff class, and the phpspec one is loaded via Composer so it takes precedence. So there is no production impact since you'd have to install MediaWiki in dev mode, but also it does not seem Vagrant-specific (other than that Vagrant installs Parsoid/PHP due to various weird dependency chains), affects all kinds of diff code and could break CI once Parsoid gets enabled there.

I'm not sure what could be done about it, other than getting rid of phpspec and/or wrapping up T166010: The Great Namespaceization and Reorg.

Upstream report: phpspec/phpspec#1262

On Vagrant, this breaks the GrowthExperiments question poster functionality (probably among other things) because the question poster triggers the Echo notification which triggers the error above, and that results in xdebug output being added to the API response.

We should get rid of phpspec since that was a short-lived experiment and looks like we've only used phpunit since. Do you want to port over those two test files? :-)

ssastry edited projects, added Parsoid-PHP; removed Parsoid.Sep 27 2019, 2:16 PM

ssastry triaged this task as Medium priority.Sep 27 2019, 4:13 PM

Change 540440 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/services/parsoid@master] Remove phpspec

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

gerritbot added a project: Patch-For-Review.Oct 2 2019, 4:41 PM

ssastry assigned this task to Tgr.Oct 7 2019, 8:13 PM

@Tgr, please test with master version of Parsoid after the above patch merges and resolve this ticket if the issue is resolved.

Change 540440 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Remove phpspec