Skip to content

MW 1.23 / Redirects disappear after the UpdateJob was performed #212

Closed
mwjames opened this Issue Feb 26, 2014 · 14 comments

4 participants

@mwjames
mwjames commented Feb 26, 2014

Moved discussion from #202:

OK, I found out that on a MW 1.22.1 (SQLite) + MW 1.22.2 (MySQL) with the current master the issue (disappearing of redirect annotations after a UpdateJob run) does not appear while on a 1.23alpha (4f60729 + 806df07) (MySQL) the issue does emerge.

@mwjames mwjames added the bug label Feb 26, 2014
@mwjames
mwjames commented Feb 26, 2014
  • Create a single page, add a simple #ask such as {{#ask:[[Modification date::+]]|?Modification date |format=broadtable}}
  • Move page to a different target (with Leave a redirect behind option)
  • Special:Browse the new page and find the redirect page annotation
  • Use Special:Admin to initiate Data repair and upgrade, and runJobs
  • Special:Browse the redirected page again and confirm that the redirect page annotation exists
@mwjames
mwjames commented Feb 26, 2014

kghbln wrote

Affirmative, with MW 1.22.2, SMW 1.9.1.1-alpha and MySQL 5.1.66 I get the redirect page annotation, with MW 1.23-alpha (2d5cc64) I do not get it.

@mwjames
mwjames commented Feb 26, 2014

I get the redirect page annotation, with MW 1.23-alpha (2d5cc64) I do not get it.

Since this only occurs on the MW 1.23 release, I thought it has something to do with the ContentHandler therefore I deactivated ContentHandler in SMW\ContentParser, which then uses the good old fashion $this->parser->parse instead and et voilà the correct ParserOutput is fetched and feed into the parser hook that makes up the annotation.

protected function hasContentHandler() {
    return false;
}
$this->parserOutput = $this->parser->parse(
    $this->getRevision()->getText(),
    $this->getTitle(),
    $this->makeParserOptions(),
    true,
    true,
    $this->getRevision()->getID()
);

This questions is what to do now?

  • Do we leave this since MW 1.23 is not yet officially released?
  • Do we ask people not to use MW 1.23 until this issue is solved?

The SMW\ContentParser does nothing fancy just what is expected to get the ParserOutput and it works except for MW 1.23 (well it worked on some earlier versions of MW 1.23 but apparently something has changed).

$content = $this->getRevision()->getContent( Revision::RAW );

if ( !$content ) {
    $content = $this->getRevision()->getContentHandler()->makeEmptyContent();
}

$this->parserOutput = $content->getParserOutput(
    $this->getTitle(),
    $this->getRevision()->getId(),
    null,
    true
);
@mwjames mwjames changed the title from MW 1.23 / Use of ContentHandler::makeContent( ... )->getRedirectTarget(); to MW 1.23 / After UpdateJob redirects are missing Mar 18, 2014
@mwjames mwjames changed the title from MW 1.23 / After UpdateJob redirects are missing to MW 1.23 / After UpdateJob run redirects are missing Mar 18, 2014
@mwjames mwjames changed the title from MW 1.23 / After UpdateJob run redirects are missing to MW 1.23 / Redirects are missing after UpdateJob was performed Mar 19, 2014
@mwjames mwjames changed the title from MW 1.23 / Redirects are missing after UpdateJob was performed to MW 1.23 / Redirects are missing after the UpdateJob was performed Mar 19, 2014
@mwjames mwjames added the analysis label Mar 20, 2014
@mwjames mwjames added a commit that referenced this issue Mar 20, 2014
@mwjames mwjames Add SimplePageRedirectRegressionTest
Adds a minimal regression test for redirects. A quick test to avoid
failures as seen by #230 due to missing test coverage.

Relates to #231, #212
8761530
@thingles
Semantic MediaWiki member

FYI @hexmode

@mwjames
mwjames commented Apr 29, 2014

@kghbln Any thoughts?

SimplePageRedirectRegressionTest currently does not test/catch the particular reported issue and I can't see me at the moment to write a test that catches the ParserOutput issue during the InternalParseBeforeLinks hook.

As stated in an earlier comment, a work around is not to use the ContentHandler in SMW\ContentParser but the code in question works for MW 1.21/1.22 and is only broken starting with MW 1.23 (in some early 1.23 version it still worked) therefore introducing a hack should be the last resort.

Until someone can confirm (#212 (comment) ) that the issue is fixed (or no longer exists), or someone writes a test to simulate the issue, I can't really recommend MW 1.23 for SMW 1.9+.

http://lists.wikimedia.org/pipermail/wikitech-l/2014-April/076188.html

@kghbln
Semantic MediaWiki member
kghbln commented Apr 29, 2014

I have just tested again according to #212 (comment) on MW 1.23.0rc0 and can confirm that the redirect annotation still disappeares (original page). I am using SMW 1.9.3 alpha (d99a162).

To have things work with MW 1.23 will be very nice since this is a LTS supported till May 2017.

@mwjames mwjames changed the title from MW 1.23 / Redirects are missing after the UpdateJob was performed to MW 1.23 / Redirects disappear after the UpdateJob was performed Apr 29, 2014
@mwjames mwjames added the mediawiki label Apr 29, 2014
@mwjames
mwjames commented May 1, 2014

or someone writes a test to simulate the issue

@hexmode It seems that WMF employees are quick in pointing the finger at others [0] therefore against my personal time constraint, I wrote a test [1] to verify that expected results do not match up when running against MW 1.22 and MW 1.23. If WMF still insists that 62856 is invalid even though the test proves otherwise nothing much I can do and I will refrain from spending more time on this issue.

[0] http://lists.wikimedia.org/pipermail/wikitech-l/2014-April/076248.html

[1] #295

@mwjames
mwjames commented May 1, 2014

I can't really recommend MW 1.23 for SMW 1.9+

@thingles @kghbln @JeroenDeDauw The SMW independent integration test #295 clearly fails for MW 1.23/MW 1.24 therefore I can't recommend MW 1.23 or MW 1.24 for production use.

@thingles
Semantic MediaWiki member
thingles commented May 2, 2014

I've already leaped to 1.23, and not sure how easy it is to go back. I very rarely use redirection on my wikis so I’m hoping this isn't a big problem for me? Things seem okay…

Editorial: The MediaWiki core team sure could do a better job working with extension developers. I think I can say that nearly all the core bugs I've seen highlighted by Semantic MediaWiki are by default rejected until it’s proven to be a problem. if you compare the support extension developers get in the MediaWiki ecosystem to that in WordPress, it’s amazing how much poorer it is. It’s disheartening for the entire ecosystem. :-( (/cc @hexmode, I know this is something you care about)

@mwjames
mwjames commented May 2, 2014

@hexmode I'm starting to loose track here because now WMF argues that all known behaviour until MW 1.22 was incorrect and is actually fixed in MW 1.23 and even suggest that #REDIRECT line shouldn't be being parsed at all and shouldn't be included in the wikitext passed to the parser.

(In reply to MWJames from comment 14)
Of course, if you are telling me that this is the expected behaviour then
naturally you can argue that MW 1.21/MW 1.22 were wrong in the first place
and that MW 1.23 has been fixed.

Yes, that's exactly what I'm telling you.

@mwjames
mwjames commented May 3, 2014

While a proposal 131210 was made to change the behavior for MW 1.24 it will probably be only SMW 1.9.4 or 1.9.5 that will support this since I have to re-write several unit/integration tests together with the interface for the InternalParseBeforeLinks hook, InTextAnnotationProcessor, and RedirectPropertyAnnotator.

/**
 * Bug: 62856
 */
protected function getRedirectTarget() {

    if ( $this->hasRedirectTargetOnParserOptions() ) {
        return $this->parser->getOptions()->getRedirectTarget();
    }

    return Title::newFromRedirect( $this->text );
}

/**
 * FIXME Remove internal check once MW 1.24 becomes mandatory
 */
private function hasRedirectTargetOnParserOptions() {
    return method_exists( $this->parser->getOptions(), 'getRedirectTarget' );
}
@JeroenDeDauw
Semantic MediaWiki member

@mwjames So you fixed this?

@mwjames
mwjames commented Aug 19, 2014

So you fixed this?

It isn't fixed the way I wanted but instead we use a workaround to circumvent the ContentHandler and rely on the good old Parser object.

Keeping this open to monitor the issue.

@mwjames mwjames added a commit that referenced this issue Oct 30, 2014
@mwjames mwjames Fix circular reference for redirects (reverts #204)
What thought as an simple precaution against a non-updated redirect (which
might have been caused by the MW issue outlined in #212) create a circular reference.

- Create page `A` to #REDIRECT [[B]]
- Create page `B` to #REDIRECT [[C]]
- Create page `C` to #REDIRECT [[A]]

Above creation schema will create an infite UpdateJob Queue.
541f8f5
@mwjames mwjames added a commit that referenced this issue Oct 30, 2014
@mwjames mwjames Fix circular reference for redirects created by UpdateJob (reverts #204)
What thought as an simple precaution against a non-updated redirect (which
might have been caused by a MW issue outlined in #212) can create a circular reference.

- Create page `A` to `#REDIRECT [[B]]`
- Create page `B` to `#REDIRECT [[C]]`
- Create page `C` to `#REDIRECT [[A]]`

Above creation schema will create an infinite `UpdateJob` Queue.
ab5b504
@mwjames mwjames closed this Jan 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.