Page MenuHomePhabricator
Create Task
Maniphest T364569

ContributionsSpecialPage/ContributionsPager introduction breaks CI
Closed, ResolvedPublic
Actions

Description

https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1028873/2
SpecialContributions__getForm__filtersHook::onSpecialContributions__getForm__filters() param type of $sp was WIDENED to ContributionsSpecialPage
same with SpecialContributionsBeforeMainOutputHook::onSpecialContributionsBeforeMainOutput()

https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1025467/5
ContribsPager__getQueryInfoHook::onContribsPager__getQueryInfo() param type of $pager was WIDENED to ContributionsPager, same with

  • ContribsPager__reallyDoQueryHook::onContribsPager__reallyDoQuery()
  • ContributionsLineEndingHook::onContributionsLineEnding()

These are breaking changes, e.g. preventing ORES merges
https://gerrit.wikimedia.org/r/c/mediawiki/extensions/ORES/+/1029513

12:45:51 includes/Hooks/ContributionsHooksHandler.php:42 PhanParamSignatureMismatch Declaration of function onContribsPager__getQueryInfo(\MediaWiki\Pager\ContribsPager $pager, array &$query) : bool|void should be compatible with function onContribsPager__getQueryInfo(\MediaWiki\Pager\ContributionsPager $pager, array &$queryInfo) : bool|void defined in ../../includes/specials/Hook/ContribsPager__getQueryInfoHook.php:25 (Expected \MediaWiki\Pager\ContribsPager $pager to have the same type as \MediaWiki\Pager\ContributionsPager $pager or a supertype)
12:45:51 includes/Hooks/ContributionsHooksHandler.php:120 PhanParamSignatureMismatch Declaration of function onSpecialContributions__getForm__filters(\MediaWiki\Specials\SpecialContributions $page, array[] &$filters) : bool|void should be compatible with function onSpecialContributions__getForm__filters(\MediaWiki\SpecialPage\ContributionsSpecialPage $sp, array &$filters) : bool|void defined in ../../includes/specials/Hook/SpecialContributions__getForm__filtersHook.php:27 (Expected \MediaWiki\Specials\SpecialContributions $page to have the same type as \MediaWiki\SpecialPage\ContributionsSpecialPage $sp or a supertype)

Details

SubjectRepoBranchLines +/-
HomepageHooks: Inherit documentation for contributions hookmediawiki/extensions/GrowthExperimentsmaster+1 -4
Hooks.php: Prepare for contributions refactormediawiki/extensions/Flowmaster+9 -1
Hooks.php: Fix documentation for contribution pages hook handlersmediawiki/extensions/Flowmaster+2 -14
ContributionsHooksHandler: Inherit documentation from interfacesmediawiki/extensions/ORESmaster+8 -6
Revert "Make ContributionsSpecialPage parent for SpecialContributions"mediawiki/coremaster+837 -970
Revert "Add ContributionsPager, an abstract parent for ContribsPager"mediawiki/coremaster+719 -816
Revert "ContribsPager: Refactor getQueryInfo for extending"mediawiki/coremaster+24 -32
Customize query in gerrit

Related Objects
Search...

Event Timeline

DannyS712 created this task.Thu, May 9, 5:27 PM
Restricted Application added a project: User-DannyS712. · View Herald TranscriptThu, May 9, 5:27 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
DannyS712 triaged this task as Unbreak Now! priority.Thu, May 9, 5:27 PM
gerritbot added a comment.Thu, May 9, 5:28 PM

Change #1029553 had a related patch set uploaded (by DannyS712; author: DannyS712):

[mediawiki/core@master] Revert "Add ContributionsPager, an abstract parent for ContribsPager"

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

gerritbot added a project: Patch-For-Review.Thu, May 9, 5:28 PM

Change #1029554 had a related patch set uploaded (by DannyS712; author: DannyS712):

[mediawiki/core@master] Revert "ContribsPager: Refactor getQueryInfo for extending"

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

gerritbot added a comment.Thu, May 9, 5:29 PM

Change #1029555 had a related patch set uploaded (by DannyS712; author: DannyS712):

[mediawiki/core@master] Revert "Make ContributionsSpecialPage parent for SpecialContributions"

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

DannyS712 added subscribers: Tchanders, Jdforrester-WMF, STran.Thu, May 9, 5:31 PM
Jdforrester-WMF added a comment.Thu, May 9, 5:31 PM

Bah, yeah, I was wondering whether we should making the breaking change in the other direction (where CI passes fine but actually it's broken as hell). I don't think either approach is great, but this one seemed the less-bad.

matmarex subscribed.Thu, May 9, 6:00 PM

Note that this is only a Phan warning. It's not a breaking change for the PHP code; it still works correctly. That's because the definitions of these hooks do not have PHP type annotations, only PHPDoc annotations, and I vaguely recall someone saying that this was done on purpose specifically to allow this kind of changes without them technically being breaking changes. (I even happen to have a patch pending that would take advantage of that to make a not-technically-breaking change: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/999129.)

Does anyone remember this less vaguely, or perhaps knows where it's written down?

taavi subscribed.Thu, May 9, 6:03 PM
Zabe subscribed.Thu, May 9, 7:18 PM
gerritbot added a comment.Thu, May 9, 7:39 PM

Change #1029553 merged by jenkins-bot:

[mediawiki/core@master] Revert "Add ContributionsPager, an abstract parent for ContribsPager"

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

gerritbot added a comment.Thu, May 9, 7:39 PM

Change #1029554 merged by jenkins-bot:

[mediawiki/core@master] Revert "ContribsPager: Refactor getQueryInfo for extending"

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

gerritbot added a comment.Thu, May 9, 7:40 PM

Change #1029555 merged by jenkins-bot:

[mediawiki/core@master] Revert "Make ContributionsSpecialPage parent for SpecialContributions"

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

Maintenance_bot removed a project: Patch-For-Review.Thu, May 9, 8:30 PM
kostajh subscribed.Thu, May 9, 9:17 PM
Umherirrender subscribed.Thu, May 9, 9:34 PM
In T364569#9784186, @matmarex wrote:

Note that this is only a Phan warning. It's not a breaking change for the PHP code; it still works correctly. That's because the definitions of these hooks do not have PHP type annotations, only PHPDoc annotations, and I vaguely recall someone saying that this was done on purpose specifically to allow this kind of changes without them technically being breaking changes. (I even happen to have a patch pending that would take advantage of that to make a not-technically-breaking change: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/999129.)

Does anyone remember this less vaguely, or perhaps knows where it's written down?

Maybe that is from T240307#6191788

matmarex added a subscriber: daniel.Thu, May 9, 10:09 PM
In T364569#9784717, @Umherirrender wrote:
In T364569#9784186, @matmarex wrote:

Note that this is only a Phan warning. It's not a breaking change for the PHP code; it still works correctly. That's because the definitions of these hooks do not have PHP type annotations, only PHPDoc annotations, and I vaguely recall someone saying that this was done on purpose specifically to allow this kind of changes without them technically being breaking changes. (I even happen to have a patch pending that would take advantage of that to make a not-technically-breaking change: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/999129.)

Does anyone remember this less vaguely, or perhaps knows where it's written down?

Maybe that is from T240307#6191788

Thanks, that comment from @daniel may be what I remembered! I think that lends some credence to the idea of making changes like the ones in this task. Maybe we'll hear from the man himself what he thinks these days, also being the main author of the stable interface policy. ;)

Tchanders added a comment.EditedMon, May 13, 9:01 AM

I had a look at all the hook handlers for the SpecialContributions and ContribsPager hooks. Most of them use @inheritDoc or document a SpecialPage or IContextSource, but a few need updating.

SpecialContributions hooks

ContributionsBeforeMainOutput:

ContributionsToolLinks:

  • No updates needed

SpecialContributions__getForm__filters:

ContribsPager hooks

ContribsPager__reallyDoQuery:

ContribsPager__getQueryInfo:

SpecialContributions__formatRow__flags:

  • Doesn't pass the page

ContributionsLineEnding:

  • No updates needed
Tchanders claimed this task.Mon, May 13, 9:04 AM
Tchanders added a project: Trust and Safety Product Sprint (Sprint Shekere (13th May - 24th May)).
Tchanders moved this task from Priority Backlog to In Progress on the Trust and Safety Product Sprint (Sprint Shekere (13th May - 24th May)) board.Mon, May 13, 9:27 AM
Tchanders added a comment.Mon, May 13, 11:13 AM

There are also some non-WMF extensions that would need updating:

I think these can be beyond the scope of this task, but we do need to make sure we update the change notes in T363358: Create extendable parent classes for special pages showing contributions lists.

gerritbot added a comment.Mon, May 13, 11:15 AM

Change #1030911 had a related patch set uploaded (by Tchanders; author: Tchanders):

[mediawiki/extensions/ORES@master] ContributionsHooksHandler: Inherit documentation from interfaces

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

gerritbot added a project: Patch-For-Review.Mon, May 13, 11:15 AM

Change #1030912 had a related patch set uploaded (by Tchanders; author: Tchanders):

[mediawiki/extensions/Flow@master] Hooks.php: Fix documentation for contribution pages hook handlers

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

gerritbot added a comment.Mon, May 13, 11:16 AM

Change #1030913 had a related patch set uploaded (by Tchanders; author: Tchanders):

[mediawiki/extensions/Flow@master] Hooks.php: Prepare for contributions refactor

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

gerritbot added a comment.Mon, May 13, 12:57 PM

Change #1030945 had a related patch set uploaded (by Tchanders; author: Tchanders):

[mediawiki/extensions/GrowthExperiments@master] HomepageHooks: Inherit documentation for contributions hook

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

gerritbot added a comment.Mon, May 13, 1:32 PM

Change #1030911 merged by jenkins-bot:

[mediawiki/extensions/ORES@master] ContributionsHooksHandler: Inherit documentation from interfaces

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

ReleaseTaggerBot added a project: MW-1.43-notes (1.43.0-wmf.5; 2024-05-14).Mon, May 13, 2:00 PM
Tchanders moved this task from In Progress to Needs Review on the Trust and Safety Product Sprint (Sprint Shekere (13th May - 24th May)) board.Mon, May 13, 2:43 PM
gerritbot added a comment.Mon, May 13, 4:07 PM

Change #1030912 merged by jenkins-bot:

[mediawiki/extensions/Flow@master] Hooks.php: Fix documentation for contribution pages hook handlers

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

gerritbot added a comment.Mon, May 13, 4:07 PM

Change #1030913 merged by jenkins-bot:

[mediawiki/extensions/Flow@master] Hooks.php: Prepare for contributions refactor

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

gerritbot added a comment.Mon, May 13, 4:35 PM

Change #1030945 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] HomepageHooks: Inherit documentation for contributions hook

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

Maintenance_bot removed a project: Patch-For-Review.Mon, May 13, 5:30 PM
matmarex added a parent task: T363358: Create extendable parent classes for special pages showing contributions lists.Wed, May 15, 12:51 PM
matmarex closed this task as Resolved.Wed, May 15, 12:58 PM
matmarex mentioned this in T365001: Clarify stable interface policy: do the stability promises apply to PHPDoc annotations?.

Looks like this is done. I filed T365001: Clarify stable interface policy: do the stability promises apply to PHPDoc annotations? about clarifying the documentation.

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL