Page MenuHomePhabricator

I09a178e5c6938954edb2949f13660227d6a01fbc breaks extension Semantic MediaWiki
Closed, ResolvedPublic

Description

Gerrit change 533002 breaks at least one extension (Semantic MediaWiki). See https://travis-ci.org/SemanticMediaWiki/SemanticResultFormats/jobs/597263289#L1377

Please revert 533002 for now (and follow deprecation procedures, if you want to re-introduce it). See https://www.mediawiki.org/wiki/Deprecation_policy.

@mwjames @JeroenDeDauw

Event Timeline

Foxtrott created this task.Oct 13 2019, 7:37 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 13 2019, 7:37 PM
Reedy added a subscriber: Reedy.Oct 14 2019, 2:16 PM

I'm struggling to see how "deprecation procedures" can necessarily be followed, as this isn't deprecation for removal, like that policy is mostly aimed towards.

I think the main missing thing is a mailing list post announcing the breaking change?

What about checking for $mTitle === null and issuing a deprecation warning?

Change 543001 had a related patch set uploaded (by Fomafix; owner: Fomafix):
[mediawiki/core@master] Revert "Parser: Add Title type hints"

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

Change 543002 had a related patch set uploaded (by Fomafix; owner: Fomafix):
[mediawiki/core@master] Parser: Add new method hasTitle() to check a Title object

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

Change 543003 had a related patch set uploaded (by Fomafix; owner: Fomafix):
[mediawiki/core@master] Parser: Add return type hint for getTitle()

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

Krinkle added a subscriber: Krinkle.EditedOct 14 2019, 8:03 PM

1.34 has not been released yet, which means no support promise has been broken yet.
I would recommend we not revert these changes but instead add the missing 1.34 release notes and announce why we found these changes necessary and deprecation was infeasible.

For future reference, I think we should also add @internal to most of Parser's methods. Its stable support promise is currently too wide to realistically support in a stable manner. Between the enactment of the deprecation policy and now, I don't think anyone has looked at Parser and intentionally decided which methods should be public. The current public methods are mostly just accidentally public due to it being the default previously for internal or testing needs (back from the PHP4 era). Those should also be in the release notes as breaking changes (for 1.35), and cannot follow deprecation either.

You can't have a cake and eat it too: T228881: "PageTranslationHooks.php": Call to a member function getIsSectionPreview() on null.

As a summary I think this is what happened:

  • Some extensions started to rely on the documented behavior of getTitle never returning null
  • Some extensions relied on the undocumented behavior of getTitle being able to return null
  • The documentation was updated to match the actual behavior of the code
  • Follow-up patches changed the actual behavior to match the originally documented behavior
    • Currently in a mixed state where getTitle is type-hinted to return Title, but this is not actually enforced

Wouldn't merging https://gerrit.wikimedia.org/r/c/mediawiki/core/+/533559 also solve this issue?

https://gerrit.wikimedia.org/r/c/mediawiki/core/+/533559 would probably also solve the issue, although I am not sure I like this kind of special value. I think introducing a hasTitle() method would be preferable.
In any case, also this would need changes in extensions that currently rely on an is_null() check, so also for this I would request a deprecation period.

(I do not consider "Removal without deprecation" as a valid alternative to each and every case of deprecation. It should be the absolute exception, not some loophole routinely used because following the proper procedure is too cumbersome. In this particular case it has been shown that deprecation could be done easily. (Thanks, @Fomafix!))

1.34 has not been released yet, which means no support promise has been broken yet.
I would recommend we not revert these changes but instead add the missing 1.34 release notes and announce why we found these changes necessary and deprecation was infeasible.

WFM.

For future reference, I think we should also add @internal to most of Parser's methods. Its stable support promise is currently too wide to realistically support in a stable manner. Between the enactment of the deprecation policy and now, I don't think anyone has looked at Parser and intentionally decided which methods should be public. The current public methods are mostly just accidentally public due to it being the default previously for internal or testing needs (back from the PHP4 era). Those should also be in the release notes as breaking changes (for 1.35), and cannot follow deprecation either.

Well, given the whole of Parser is being removed and replaced by Parsoid in a way that is a fundamental breaking change (IIRC about a dozen Parser hooks will be removed with no replacement)…

I am unable to spend a lot of mental cycles trying to figure out a good strategy here while we are heads down in getting Parsoid/PHP past the finish line and deployed soooon. So, my instinct is to simply revert this patch for now unless someone has a good recommendation for an alternative. Suggestions?

Secondly, as James said, over the next year, we are going to be undertaking much bigger changes to the parser extension/hook interface which will require most extensions to start updating their code to be Parsoid compatible and eventually (18-24 months), the core parser and its hook and extension mechanism will disappear. So, my second instinct is to be judicious about how we spend our scarce developer (both core and extension, staff or voluneer) time and energies wrt tweaking parser interfaces (and dealing with downstream impacts).

We have a second issue now: https://github.com/SemanticMediaWiki/SemanticMediaWiki/issues/4330
It's reported in SMW, but really breaks in Lingo.

So, my instinct is to simply revert this patch for now unless someone has a good recommendation for an alternative.

That seems best at this point, especially in light of . . .

Secondly, as James said, over the next year, we are going to be undertaking much bigger changes to the parser extension/hook interface which will require most extensions to start updating their code to be Parsoid compatible and eventually (18-24 months), the core parser and its hook and extension mechanism will disappear.

In addition to having the aforementioned 18-24 month transition period, it is important to have regular communications as details about the new interface become available as well as detailed migration documentation so that extensions developers can prepare for their transition to the new interface.

So, my instinct is to simply revert this patch for now unless someone has a good recommendation for an alternative.

That seems best at this point

I disagree. This is what releases are for.

I disagree. This is what releases are for.

I agree that "this is what releases are for" with a caveat: Clear communication needs to happen before breaking changes are implemented. At least one release where a behavior is deprecated should be made.

Since I do not see any deprecation, just breakage, it seems to make sense to change this to a deprecation so that developers have time to adapt.

A release is due at the end of 2019, so leaving this change until then should not cause too much problem.

cscott added a subscriber: cscott.Oct 17 2019, 4:51 PM

What about checking for $mTitle === null and issuing a deprecation warning?

FWIW I like this idea of adding a hard-deprecation notice (wfDeprecated) to the "returns null" case, and actually removing the possibility of returning null in a future release. I'm not sure we need to add the special 'bogus title' facility; it seems that extensions which want this can create the bogus title themselves, and by explicitly setting a title in every case they'll be backward-compatible with old code as well as forward-compatible with the new behavior.

Change 543903 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[mediawiki/core@master] Deprecate setting Parser::mTitle to null

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

So to be concrete, I'm suggesting to merge @Fomafix's revert https://gerrit.wikimedia.org/r/543001 and then my deprecation patch on top of that https://gerrit.wikimedia.org/r/543903 but *not* the hasTitle and return type hint patches https://gerrit.wikimedia.org/r/543002 and https://gerrit.wikimedia.org/r/543003.

Krinkle removed a subscriber: Krinkle.Oct 17 2019, 6:12 PM

Sounds good to me. Sorry about the confusion.

While I am not convinced that the original change falls under the deprecation policy, I can go with @cscott's proposal since a lot of Parser.php code is slated for deprecation as we proceed with Parsoid integration.

Just to be super clear so this doesn't set a precedent for similar future changes, breaking changes like this aren't deprecations. In this case, deprecation makes sense for pragmatic reasons since this is mostly a cleanup and we can roll it with other deprecations coming over the next year. Merging revert and @cscott's changes now.

Change 543001 merged by jenkins-bot:
[mediawiki/core@master] Revert "Parser: Add Title type hints"

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

Change 544227 had a related patch set uploaded (by Reedy; owner: Fomafix):
[mediawiki/core@REL1_34] Revert "Parser: Add Title type hints"

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

Change 544249 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[mediawiki/core@master] Parser: Add Title type hints

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

Change 543903 merged by jenkins-bot:
[mediawiki/core@master] Deprecate setting Parser::mTitle to null

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

Thanks to everybody who helped to solve this issue. Much appreciated!

Change 544227 merged by jenkins-bot:
[mediawiki/core@REL1_34] Revert "Parser: Add Title type hints"

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

Change 543002 abandoned by Fomafix:
Parser: Add new method hasTitle() to check a Title object

Reason:
Superseded by Ia254949cd8b3bc162b11dcc911dcce40d91bf1b7

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

Change 544258 had a related patch set uploaded (by Reedy; owner: C. Scott Ananian):
[mediawiki/core@REL1_34] Deprecate setting Parser::mTitle to null

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

Change 544258 merged by jenkins-bot:
[mediawiki/core@REL1_34] Deprecate setting Parser::mTitle to null

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

Reedy added a comment.Oct 18 2019, 9:38 PM

This is all backported

I'm a bit surprised that change to make the method function like documented (and, imho, what extension developers could reasonably expect) is considered a breaking change and not the unannounced change that changed documented behavior to match the actual behavior. In any case, T228881: "PageTranslationHooks.php": Call to a member function getIsSectionPreview() on null will eventually be fixed after deprecation period so we will get actual behavior that matches the documented behavior.

This is all backported

Thanks @Reedy.

Well, 1.34.0 is due in November, so it seems we don't actually have to wait that long to fix T228881. In fact, since 1.34 has already forked and @Reedy has done the backports, it could be landed on master now. I'd prefer waiting a few weeks just to let the deprecation code ride the train and get testing and make sure @Fomafix and I haven't inadvertently regressed something with our revert + deprecation patches, but early November seems reasonable to land https://gerrit.wikimedia.org/r/544249.

Change 543003 abandoned by Fomafix:
Parser: Add return type hint for getTitle()

Reason:
Superseded by I076ae0dbfbbb8be731367e4641f9c8aacf7586e9.

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

Reedy closed this task as Resolved.Nov 4 2019, 6:26 PM
Reedy removed a project: Patch-For-Review.

Change 544249 merged by jenkins-bot:
[mediawiki/core@master] Parser: Add Title type hints

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