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.
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?
Change 543001 had a related patch set uploaded (by Fomafix; owner: Fomafix):
[mediawiki/core@master] Revert "Parser: Add Title type hints"
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
Change 543003 had a related patch set uploaded (by Fomafix; owner: Fomafix):
[mediawiki/core@master] Parser: Add return type hint for getTitle()
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:
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!))
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.
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.
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.
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
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.
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"
Change 544227 had a related patch set uploaded (by Reedy; owner: Fomafix):
[mediawiki/core@REL1_34] Revert "Parser: Add Title type hints"
Change 544249 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[mediawiki/core@master] Parser: Add Title type hints
Change 543903 merged by jenkins-bot:
[mediawiki/core@master] Deprecate setting Parser::mTitle to null
Change 544227 merged by jenkins-bot:
[mediawiki/core@REL1_34] Revert "Parser: Add Title type hints"
Change 543002 abandoned by Fomafix:
Parser: Add new method hasTitle() to check a Title object
Reason:
Superseded by Ia254949cd8b3bc162b11dcc911dcce40d91bf1b7
Change 544258 had a related patch set uploaded (by Reedy; owner: C. Scott Ananian):
[mediawiki/core@REL1_34] Deprecate setting Parser::mTitle to null
Change 544258 merged by jenkins-bot:
[mediawiki/core@REL1_34] Deprecate setting Parser::mTitle to null
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.
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.
Change 544249 merged by jenkins-bot:
[mediawiki/core@master] Parser: Add Title type hints