Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

return types were incorrectly hinted as null, should be void #49

Merged
merged 1 commit into from
Oct 10, 2016
Merged

return types were incorrectly hinted as null, should be void #49

merged 1 commit into from
Oct 10, 2016

Conversation

mindplay-dk
Copy link
Contributor

Corrected all return type-hints from null to void to avoid failed inspections in e.g. Php Storm.

(strict inspection would state that a function must return if annotated with return null - as opposed to return void, which means the function does not use a return-statement and has no defined return-value.)

@GrahamCampbell
Copy link
Contributor

This is technically a breaking change to the interfaces.

@Seldaek
Copy link
Collaborator

Seldaek commented Sep 23, 2016

It's not really though, functionally speaking, and as we have no type hints, it doesn't matter at all. I'd be +1 for merging here, seeing as PHP7.1 made void official, but to merge the change on the PSR itself I guess you'll have to go through the ML.

@michaelcullum
Copy link
Member

Please send a message to the mailing list if anyone objects, as this might be considered a breaking change; if nobody does object then we can get this merged here and on the spec. If people do object then it can either go to a vote (if people care enough either way) or it needs to remain as-is here and on the spec.

@mindplay-dk
Copy link
Contributor Author

This is technically a breaking change to the interfaces.

Technically, but in practice, no one has been writing return null statements at the end of every method implementation.

So in practice, this is not a breaking change, it's a bugfix.

For that matter, it's not even a bugfix, it's a documentation fix :-)

@michaelcullum
Copy link
Member

Sure, it's just it's better to make sure on the ML nobody objects first. :)

@GrahamCampbell
Copy link
Contributor

It's not really though, functionally speaking, and as we have no type hints, it doesn't matter at all.

That thing is really a typehint documentation wise. It says all implementations must return null, but changing it to void means all implementations must not return. See why I'd say this is breaking?

@Seldaek
Copy link
Collaborator

Seldaek commented Sep 27, 2016

That's quite a pedantic argument though, given that return; and return null; are 99% equivalent in PHP, and that it would not break anything at all in userland.

@mindplay-dk
Copy link
Contributor Author

See why I'd say this is breaking?

@GrahamCampbell yes, but show me one implementation that returns? It's technically a break change, and I already said as much, but since there's not a line of code actually implementing it "correctly" according to the incorrect doc-block, changing this effectively breaks nothing.

@GrahamCampbell
Copy link
Contributor

Yeh. I'm just playing devil's advocate. I do think this change is fine. I just want to make sure all points are properly considered. :)

@mindplay-dk
Copy link
Contributor Author

By the way, it just occurred to me - the included AbstractLogger, LoggerTrait and NullLogger implementations also do not implement the interfaces according to their own annotations.

So this really is a bugfix.

@Seldaek
Copy link
Collaborator

Seldaek commented Oct 10, 2016

OK I think we all agree here this is insignificant so merging, we wasted enough time on this IMO.

@Seldaek Seldaek merged commit 4ebe3a8 into php-fig:master Oct 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants