On Thu, May 7, 2020 at 12:43 PM Nicolas Grekas <nicolas.grekas+...@gmail.com> wrote:
> Hi Benjamin, > > https://wiki.php.net/rfc/deprecated_attribute > > > Thanks for the RFC. > Why didn't you mention support for deprecating classes/interfaces/traits > themselves? > If there is a reason, it should be in the RFC at least to me. > It feels complicated to me, because there are so many entry points to a class: 0. declaring/loading it 1. extending it, but not always because the replacement could extend it 2. calling it statically 3. instantiating it 4. i probably forget a bunch. I think this requires spreading the code to handle this around into many places and is not easy to do without overhead for classes that are *not* themselves deprecated. So I decided to keep this out for now. > - all other languages with Attributes/Annotations support have it as well >> > > Did you research what runtime side effects the other languages provide? > They mostly warn or fail on compile and don't have runtime effects. The same goes for any other deprecation that PHP triggers though, they are all at runtime. So I don't think this can be compared, similar to how the error stack of PHP cannot be compared to any other language. > > I'm concerned about runtime side-effects. Attributes should lead to none > of them to me. The moment an attribute leads to runtime side effects, it > should be turned into code instead, because it looses its "declarative" > property. > To me Symfony using Annotations at the moment is also all about side effects based on declared annotations that are then automatically turned into code. This is the same happening here. The difference to this example here https://symfony.com/doc/current/bundles/SensioFrameworkExtraBundle/annotations/security.html#usage is only that <<Deprecated>> will generate the code into the function/method, whereas the annotations in the Symfony example generate the code "around" (before) the method call. > > From my experience, e.g. a method can be declared as deprecated but can > *still* be called internally without any issue. This should not be reported > to anyone as its internal. Preserving BC often requires this, and only > runtime logic can decide when a deprecation should be reported to the > end-user. Note that this is different from configuring the global error > reporting level: deciding *locally* that some declaration should not be > reported is the important part of this. > Can I ask how you are handling this use-case in Symfony at the moment? If you catch that in the error handler by looking at the backtrace, then this is exactly possible with this declarative approach as well, in fact the generated code is just calling trigger_error() and then triggering user error handlers. > > That's also why I very much favor @trigger_error() and why static analysis > works only for the simpler cases. > > If one wants to save the duplicate "declaration" (attribute + explicit > call inline), then this should be opt-in to me, e.g. <<Deprecated(true)>>. > If you only want the static analysis effect, developers are still free to use /** @deprecated */ instead, which I don't expect either static analysis tools nor IDEs will drop support for. > > As an additional step, I very much which that the attribute could accept > two arguments: the package and the version of it. From my experience, > making deprecations actionable is critical to help ppl to fix them. And > making them actionable starts with allowing ppl to quickly identify which > package cannot be upgraded to the next major yet. > > This is what make us work on symfony/deprecation-contracts > <https://github.com/symfony/deprecation-contracts>, which provide this > single trigger_deprecation(string $package, string $version, string $ > message, ...$args); function. From the example in the readme: > > trigger_deprecation('symfony/blockchain', '8.9', 'Using "%s" is deprecated, > use "%s" instead.', 'bitcoin', 'fabcoin'); > > Generates: > Since symfony/blockchain 8.9: Using "bitcoin" is deprecated, use "fabcoin" > instead. > > Joke aside, this is very useful and I wish the attribute could borrow from > the experience we gathered on the topic. > > This would be nice: > <<Deprecated('symfony/blockchain', '8.9', 'Using "%s" is deprecated, use "%s" > instead.', 'bitcoin', 'fabcoin')>> > > The Deprecated attribute accepts a message that gets appended to the default message, so you could do <<Deprecated("since symfony/blockchain 8.9, use fabcoin() instead")>>, which would turn the message into: Deprecated: Function bitcoin() is deprecated, since symfony/blockchain 8.9, use fabcoin() instead in /path/to/file:123 A free-form text is more flexible for use-cases where there are no package and versions, we can't expect to assume every deprecation will be triggered from a versioned composer package only. > > It could also be quiteuseful to have this trigger_deprecation() function in > core, but that's another topic :) > > Yes, a declarative statement like <<Deprecated>> would have the benefit of keeping the implementation hidden, so details of it could potentially be changed. > > Cheers, > > Nicolas > >
