Skip to content

Conversation

Ayesh
Copy link
Member

@Ayesh Ayesh commented Jun 1, 2021

Currently interface methods with visibility private or protected fail with an error message:

Access type for interface method A::b() must be omitted

However, explicitly setting visibility public is allowed and often desired. This commit updates the error message to:

Access type for interface method A::b() must be omitted or declared public

@Ayesh Ayesh marked this pull request as ready for review June 1, 2021 23:33
@krakjoe
Copy link
Member

krakjoe commented Jun 2, 2021

I think "must be public" would be better.

Currently interface methods with visibility `private` or `protected` fail
with an error message:

  Access type for interface method A::b() must be omitted

However, explicitly setting visibility `public` is allowed and often desired.
This commit updates the error message to:

  Access type for interface method A::b() must be public
@Ayesh Ayesh force-pushed the interface-visibility-error-msg branch from 6022a24 to 5340bac Compare June 2, 2021 17:24
@Ayesh
Copy link
Member Author

Ayesh commented Jun 2, 2021

Thank you @krakjoe - I just force-pushed with the updated error message.

@Girgias Girgias merged commit a706d73 into php:master Jun 2, 2021
@Ayesh Ayesh deleted the interface-visibility-error-msg branch June 2, 2021 19:10
@Ayesh
Copy link
Member Author

Ayesh commented Jun 2, 2021

Thank you @Girgias.

@kocsismate
Copy link
Member

I am a bit late to the party, but I wanted to suggest to also deal with the "access type" part of the message. In my opinion, "Visibility of interface method..." would be more consistent with the terminology used in the documentation (https://www.php.net/manual/en/language.oop5.visibility.php) - although it also heavily uses "access", so neither "access type" is that bad after all, I guess.

@kocsismate
Copy link
Member

Actually, I was also working on improving error messages last year, and I got some inspiration from one of my old PRs: https://github.com/php/php-src/pull/6385/files#diff-7eccab80ba3e1beb270eb11da8c9642103ed12f3abb12e0acc71b8e344f0eac3R14:

Interface method A::b() must have a public visibility

This is the version which sounds the most natural for me.

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