On Wed, Apr 24, 2024 at 3:57 PM Nicolas Grekas <nicolas.grekas+...@gmail.com> wrote:
> Hi Benjamin, > > My PR for #[\Deprecated] attribute was in hibernation for a long while now >> and after some off-list discussion a few weeks ago I have decided to >> revisit it and asked Tim to help me out with the work. >> >> Tim has cleaned up the PR quite a bit and also worked in additional >> features such as #[Deprecated] support in stub generation. >> >> While there are still some small todos, at this point we want to restart >> the discussion about the RFC for inclusion in 8.4: >> >> RFC: https://wiki.php.net/rfc/deprecated_attribute >> PR: https://github.com/php/php-src/pull/11293 >> Old discussion: https://externals.io/message/112554#112554 >> >> Let me know about your questions and feedback. >> > > Thanks for the RFC. > > While I'd like to be able to replace as many annotations as possible with > attributes, the devil is in the details and here, I'm not sold about the > details: > > - I concur with others about the "since" property being missing > > > - We'd also need a "package" property so that it's trivial to know > which composer package is deprecating. > - The attribute class should not be final, so that packages could > subclass, e.g. to define the "since" or "package" property once for all - > or to define a custom constructor, etc. > > As the RFC mentions, this is technically not possible, because the attribute is processed during compilation and an instanceof check at that point is not legal (cant autoload more during compiling). Even considering we can solve this technically, it would also otherwise secretly mean that all method/function attributes are getting autoloaded and this would break the core assumption of attributes being backed by classes on Reflection access only. > > - We should be able to add the attribute to a class. > > This is in the future scope as it requires a very different (orthogonal) implementation. > > - > > Yes, the "package" + "since" info can be put in the message itself, but > having a structured way to declare them is critical to 1. be sure that > authors don't forget to give that info and 2. build tools around that. > Are there tools around them? I can't find a use case in my head what the "since" property can be used for. Both since and package would be optional attributes, so tooling that checks they are not forgotten is needed anyways. It could just as well check for missing additional attributes next to the internal Deprecated attribute. > > You're saying that these are not useful because the engine wouldn't make > anything useful out of e.g. "since". > That's true, although I'd suggest using them to prefix the final message. > If that's the policy around attributes for the engine, then I'm wondering > if the attribute shouldn't live elsewhere. Or if we shouldn't discuss this > policy. > The attribute is there to close the gap with internal functions ZEND_ACC_DEPRECATED flag and ReflectionFunction/Method::isDeprecated already existing. This cannot live elsewhere. The since, package (and replacement) requirements however could live elsewhere. Given the many different requirements, we could think about adding an extra variadic argument, so that you could add whatever information you pass to the attributa via a key value array returned from Deprecated::getExtraInformation() ; > I also anticipate a problem with the adoption period of the attribute: in > order to be sure that a call to a deprecated method will trigger a > deprecation, authors will have to 1. add the attribute and 2. still call > trigger_error when running on PHP < 8.next. That's a lot of boilerplate. > This is a problem of any new feature that covers a use-case previously solved differently, for example attributes themselves, where libraries/Frameworks shipped both at the same time for a while. Taking this argument at face value would mean we stop evolving PHP. > > Personally, I'm not convinced that there should be any runtime > side-effects to the attribute. I'd prefer having it be just reported by > reflection. > This RFC does not introduce the runtime side effects of ZEND_ACC_DEPRECATED flag on methods or functions. They already exist with internal functions. Having this attribute not trigger the deprecation where they are already triggered for internal functions introduces an inconsistency. We should talk about how PHP implements deprecation tracking and logging as a separate issue, I have a lot of ideas here how to change that. > Nicolas > > > > >
