Page MenuHomePhabricator

Illegal string offset 'revid' in VisualEditor/includes/ApiVisualEditor.php
Closed, ResolvedPublic1 Estimate Story Points

Description

When I'm migrate to MW 1.31-rc.0, using REL1_31 (2d2c8fc) of VE, I got:

mod_fcgid: stderr: PHP Warning:  Illegal string offset 'revid' in /srv/mediawiki-1.31-rc.0/w/extensions/VisualEditor/includes/ApiVisualEditor.php on line 351, referer: https://xxx.org/w/index.php?title=%E9%A6%96%E9%A1%B5&veaction=editsource

Steps to reproduce:
Using 2017 new editor to edit any page.

Details

Related Gerrit Patches:

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 3 2018, 9:05 AM
RazeSoldier updated the task description. (Show Details)May 3 2018, 9:56 AM
RazeSoldier updated the task description. (Show Details)May 3 2018, 9:59 AM
Esanders added a subscriber: Esanders.EditedMay 3 2018, 2:30 PM

Hmm, I see no such string on line 271: https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/VisualEditor/+/2d2c8fcffe69d929489fc5c941fdad028b004b3e/includes/ApiVisualEditor.php#271

Are you sure your VE repo is correctly checked out to that version?

RazeSoldier updated the task description. (Show Details)May 4 2018, 12:04 AM

Sorry, I made a mistake before, but now the problem still exists.

RazeSoldier renamed this task from Illegal string offset 'revid' in VisualEditor/ApiVisualEditor.php to Illegal string offset 'revid' in VisualEditor/includes/ApiVisualEditor.php.May 5 2018, 5:01 PM

Are you getting the same exception? Because the exception you posted before refers to non-existent code (at least at the version you quoted).

Yes, I got the same warming. I already updated the task description.

I ran into the same issue. Here are my data:

Setup

  • MediaWiki 1.31.0-rc.0 (a3335f4) 07:55, 23. Mai 2018
  • PHP 7.0.27-0+deb9u1 (apache2handler)
  • MariaDB 10.1.26-MariaDB-0+deb9u1
  • VisualEditor 0.1.0 (2d2c8fc) 22:25, 17. Apr. 2018

Issue

2018-05-25 07:33:18 phalaris 0210020150926-02100_: [d7cc97679601ea9697499073] /w/api.php?action=visualeditor&format=json&paction=wikitext&page=Mod%C3%A8le%3ATelephone&uselang=de   ErrorException from line 351 of /../w/extensions/VisualEditor/includes/ApiVisualEditor.php: PHP Warning: Illegal string offset 'revid'

I believe that I get this for every edit. Currently this is floating the error log. However I cannot tell if this was an issue with MW 1.30 and the corresponding revision of VisualEditor.

Stack trace

#0 /../w/extensions/VisualEditor/includes/ApiVisualEditor.php(351): MWExceptionHandler::handleError(integer, string, string, integer, array)
#1 /../w/includes/api/ApiMain.php(1579): ApiVisualEditor->execute()
#2 /../w/includes/api/ApiMain.php(535): ApiMain->executeAction()
#3 /../w/includes/api/ApiMain.php(506): ApiMain->executeActionWithErrorHandling()
#4 /../w/api.php(94): ApiMain->execute()
#5 {main}

I'm not sure under what circumstances the API request would not return a revid. Perhaps you could inspect the contents of $result (on line 346) to find out what's going wrong?

I have no idea how to do this. I get this with ?veaction=editsource.

The relevant code from REL1_31 is:

if ( isset( $result['query']['pages'][$pid]['revisions'] ) ) {
	foreach ( $result['query']['pages'][$pid]['revisions'] as $revArr ) {
		if ( $revArr['revid'] === $oldid ) {
			$content = $revArr['content'];
		}
	}
}

"Illegal string offset" means that $revArr (which should be an array) is actually a string, which means (at least) one of the values of $result['query']['pages'][$pid]['revisions'] is a string.

The API request is something like https://en.wikipedia.beta.wmflabs.org/w/api.php?action=query&prop=revisions&format=json&rvprop=timestamp|user|parsedcomment|ids&rvlimit=25&pageids=7776&format=jsonfm – and briefly looking at https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/core/+/REL1_31/includes/api/ApiQueryRevisions.php I can't see how it would output a string there.

(Aside – why are we fetching all the revisions? We don't really care except for the latest revision and the one matching $oldid. Can we limit the query to load less and so be faster?)

Can one of the reporters encountering this bug use their browser's developer tool network tab to capture the output of the query (redacted if there's anything you don't want to share) so we can guess at what unexpected state we're encountering?

Change 435200 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/extensions/VisualEditor@master] ApiVisualEditor: Defensively check that query prop revisions returns arrays

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

matmarex added a subscriber: matmarex.EditedMay 25 2018, 6:45 PM

"Illegal string offset" means that $revArr (which should be an array) is actually a string, which means (at least) one of the values of $result['query']['pages'][$pid]['revisions'] is a string.
The API request is something like https://en.wikipedia.beta.wmflabs.org/w/api.php?action=query&prop=revisions&format=json&rvprop=timestamp|user|parsedcomment|ids&rvlimit=25&pageids=7776&format=jsonfm – and briefly looking at https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/core/+/REL1_31/includes/api/ApiQueryRevisions.php I can't see how it would output a string there.

I think we're seeing the internal properties the API uses to nicely format JSON or XML output. You can see them in the web API with format=rawfm: https://en.wikipedia.beta.wmflabs.org/w/api.php?action=query&prop=revisions&format=json&rvprop=timestamp|user|parsedcomment|ids&rvlimit=25&pageids=7776&format=rawfm – note the "_element": "rev" (I think this is what we get in the foreach() loop) and other properties prefixed with _.

Looks like we can ask the API to strip these for us by calling ->getResultData( null, [ 'Strip' => 'all' ] ) instead of just ->getResultData().

Apparently, the 'Illegal string offset' warning is not emitted by HHVM, so we're not running into this at WMF: https://3v4l.org/YU0dV

@matmarex Cool, thanks a lot for identifying the underlaying issue!

Kghbln added a comment.EditedMay 30 2018, 7:40 PM

I wanted to test the patch and pulled it. Thus I killed my wiki. I am not sure what is going one since this one was supposed to fix REL1_31 O_o

Change 435200 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] ApiVisualEditor: Defensively check that query prop revisions returns arrays

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

Change 436414 had a related patch set uploaded (by Bartosz Dziewoński; owner: Jforrester):
[mediawiki/extensions/VisualEditor@REL1_31] ApiVisualEditor: Defensively check that query prop revisions returns arrays

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

I wanted to test the patch and pulled it. Thus I killed my wiki. I am not sure what is going one since this one was supposed to fix REL1_31 O_o

The patch https://gerrit.wikimedia.org/r/435200 was submitted for the master branch, so depending on how you did it, you might have accidentally pulled in other changes in VE that depend on latest alpha MediaWiki.

Try to cherry-pick it instead:

git fetch https://gerrit.wikimedia.org/r/mediawiki/extensions/VisualEditor refs/changes/00/435200/2 && git cherry-pick FETCH_HEAD

Or, pull the patch for REL1_31 branch I just submitted: https://gerrit.wikimedia.org/r/436414

Change 436414 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@REL1_31] ApiVisualEditor: Defensively check that query prop revisions returns arrays

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

Kghbln closed this task as Resolved.May 31 2018, 8:40 AM
Kghbln assigned this task to Jdforrester-WMF.

@matmarex I am sorry for the confusion I caused. I though that the patch was meant to be for REL1_31 so I just used the git "download link" provided by gerrit for the patch set. This happens when you are in a hurry. :(

@Jdforrester-WMF @matmarex

To cut a long story short: I just pulled in the latest code for REL1_31 and can confirm that the issue no longer happens! Thanks a lot for fixing! Cool!

Jdforrester-WMF set the point value for this task to 1.Jun 13 2018, 6:26 PM
Vvjjkkii renamed this task from Illegal string offset 'revid' in VisualEditor/includes/ApiVisualEditor.php to 9pdaaaaaaa.Jul 1 2018, 1:12 AM
Vvjjkkii reopened this task as Open.
Vvjjkkii removed Jdforrester-WMF as the assignee of this task.
Vvjjkkii triaged this task as High priority.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed the point value for this task.
Vvjjkkii removed subscribers: gerritbot, Aklapper.
CommunityTechBot renamed this task from 9pdaaaaaaa to Illegal string offset 'revid' in VisualEditor/includes/ApiVisualEditor.php.Jul 3 2018, 12:15 AM
CommunityTechBot raised the priority of this task from High to Needs Triage.