Page MenuHomePhabricator

Undefined property: Diff::$edits in includes/diff/DiffFormatter.php
Closed, ResolvedPublic

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.

Event Timeline

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.

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 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

@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

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

I am going to mark this resolved. Please reopen / refile if there is anything else that needs fixing.