Page MenuHomePhabricator

Declaration of function onLoginFormValidErrorMessages($messages) : void should be compatible with function onLoginFormValidErrorMessages(array $messages)
Closed, ResolvedPublic

Description

21:10:54 <checkstyle version="6.5">
21:10:54   <file name="client/includes/Hooks/LoginFormValidErrorMessagesHandler.php">
21:10:54     <error line="13" severity="warning" message="Declaration of function onLoginFormValidErrorMessages(&amp;amp;$messages) : void should be compatible with function onLoginFormValidErrorMessages(array &amp;amp;$messages) (parameter #1 with no type cannot replace original parameter with type 'array') defined in ../../includes/specials/Hook/LoginFormValidErrorMessagesHook.php:22" source="PhanParamSignatureRealMismatchHasNoParamType"/>
21:10:54   </file>
21:10:54 </checkstyle>

Seemingly broken by rMW29d91c2c5716: Typehint onLoginFormValidErrorMessages param as array by @Lucas_Werkmeister_WMDE

Event Timeline

Reedy created this task.May 18 2020, 8:18 PM
Restricted Application added a project: Wikidata. · View Herald TranscriptMay 18 2020, 8:18 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Reedy updated the task description. (Show Details)May 18 2020, 8:18 PM
Reedy triaged this task as High priority.May 18 2020, 8:22 PM

Change 597157 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/Wikibase@master] Make onLoginFormValidErrorMessages match MW Core

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

Change 597157 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Make onLoginFormValidErrorMessages match MW Core

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

Reedy closed this task as Resolved.May 18 2020, 9:01 PM
Reedy claimed this task.

Ugh. Quoting myself from the commit message:

This is a compatible change for implementors, since implementing classes
are allowed to widen the parameter type from “array” to “mixed”.

I tested that PHP itself allows this, but it never occurred to me to check whether Phan allowed this or not. Sorry for the breakage :(

(So what is the correct way to upgrade an interface to use type hints if it has implementations in a different repository? Adding the type hint to the implementation first isn’t allowed by PHP because it violates LSP, but adding it to the interface first is apparently disallowed by Phan… I guess you upload changes for MediaWiki and all other repositories together, with Depends-On pointing from the other changes to the MediaWiki change, get the other changes +2ed first, and then merge the MediaWiki change, so that it all gets merged more or less at the same time?)

Reedy added a comment.May 19 2020, 1:42 PM

It happens! :)

I guess you upload changes for MediaWiki and all other repositories together, with Depends-On pointing from the other changes to the MediaWiki change, get the other changes +2ed first, and then merge the MediaWiki change, so that it all gets merged more or less at the same time?)

Yeah, I think something like that.

There's these cases where we have to break the build to fix stuff (occasionally with vendor patches, for example), it happens, unfortunately. But if the patches are lined up to go, C+2'd with depends-on, the breakage (if any at all) should only be 10-20 minutes, and usually with a quick check people can find what's broken and see that the fix is just waiting to go through the proces.

The problem in this case was that it wasn't remedied, and so I tripped over it ~8.5 hours later making a completely unrelated patch