GH-16447: improve enums' error when missing interface's hooked properties

[DanielEScherzer]

Enums cannot have properties, and so can not be made to satisfy the interface at all; show a clearer error message than the suggestion to implement the hooked properties.

[DanielEScherzer]

Note that there may be conflicts when merging this to master due to #15995 and #15993, I can work on cleaning up the logic on master afterwards but for now I put the enum check separately to try and avoid merge conflicts

[iluuu1994]

Do we need this custom error? We have other, very similar cases for forbidden methods, e.g. https://3v4l.org/ChAkh

[DanielEScherzer]

Fatal error: Enum E must implement 1 abstract private method (I::__toString) in /in/ChAkh on line 7

Though that specific method (__toString) doesn't work because Enums cannot implement Stringable, if you had a different name https://3v4l.org/1cnsv

Fatal error: Enum E must implement 1 abstract private method (I::myToString) in /in/1cnsv on line 7

this is something that the developer can actually address. On the other hand, with a hooked property

Fatal error: Enum E must implement 1 abstract private method (I::$x::get)

This is impossible and the developer cannot actually do this. If I hadn't know it was impossible, I would definitely spend time trying to figure out how to do it, and then get annoyed when I learned that it was impossible and I had wasted time.

[iluuu1994]

Though that specific method (__toString) doesn't work because Enums cannot implement Stringable, if you had a different name https://3v4l.org/1cnsv

Right, that's the point. Enums can't declare __toString or hooks, so why error early for one but not the other?

[DanielEScherzer]

Though that specific method (__toString) doesn't work because Enums cannot implement Stringable, if you had a different name https://3v4l.org/1cnsv

Right, that's the point. Enums can't declare __toString or hooks, so why error early for one but not the other?

In that case, I can also send a patch for the magic methods to not have the misleading error message - I agree we shouldn't do one but not the other, but think its better to do both than neither

[iluuu1994]

I'm not sure it's a huge benefit. I would rather not litter random error messages into the codebase without a more coherent approach. Checking for these methods during implementation of interfaces will just repeat the same checks we already do.

[DanielEScherzer]

I'm not sure it's a huge benefit. I would rather not litter random error messages into the codebase without a more coherent approach. Checking for these methods during implementation of interfaces will just repeat the same checks we already do.

So you don't want the property messages to have a warning from the interface because it isn't consistent with the magic methods, and you also don't want to have a warning for magic methods in the interface?

[DanielEScherzer]

I'm not sure it's a huge benefit. I would rather not litter random error messages into the codebase without a more coherent approach. Checking for these methods during implementation of interfaces will just repeat the same checks we already do.

So you don't want the property messages to have a warning from the interface because it isn't consistent with the magic methods, and you also don't want to have a warning for magic methods in the interface?

Bump - can you confirm if my understanding here is correct? If so I'll look into drafting a "more coherent approach"

[iluuu1994]

Optimally, we should have useful and logically consistent messages that don't require adding error messages in multiple places. That was my main concern: It's already hard to keep error messages consistent because they are littered all over the place, in a way that they can't easily be consolidated. I think we should improve that rather than making it worse.

Maybe you can re-use zend_verify_enum() rather than replicating the same rules in a different place.