Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:110076
MIME-Version: 1.0
References: <CAEPJdum26d_x6z2K4BFvRBXhNh8qQrh0tTE7wXzsfV7rwgm9Lg@mail.gmail.com>
 <CAOWwgp=qQWt+LXNAYjgWVzud+gP6LznR+LC8Pj76rueCrH6tDA@mail.gmail.com> <CAEPJdukVyPHoLTg5cnviaUGVHxzNONWCHv9SOpGqWTy30KtaVA@mail.gmail.com>
In-Reply-To: <CAEPJdukVyPHoLTg5cnviaUGVHxzNONWCHv9SOpGqWTy30KtaVA@mail.gmail.com>
Date: Fri, 8 May 2020 01:52:41 +0200
Message-ID: <CAOWwgp=--isxUN00y7mGswy9Q04QcDDMWu5Toq6y6t_RZ=vyXw@mail.gmail.com>
To: Benjamin Eberlei <kontakt@beberlei.de>
Cc: PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000003e081505a5179470"
Subject: Re: [PHP-DEV] [RFC] <<Deprecated>> Attribute
From: nicolas.grekas+php@gmail.com (Nicolas Grekas)

--0000000000003e081505a5179470
Content-Type: text/plain; charset="UTF-8"

>
> 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.
>

It is.
But the other cases are equally complex. That's the point I tried to make
in my previous answer and that I will continue to make by sharing my
experience on the topic.

That's why I'm pretty sure that runtime side-effect will severely limit the
usefulness of this attribute.
I very much wish we would adopt the tag, but without any runtime side
effect (which means in practice that php-internals doesn't need to define
it, this could be left to userland.)


- 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.
>

OK thanks.


> 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.
>

That's somewhat more subtle.

Most annotations don't lead to any direct notice. We also double these
annotations with calls to trigger_error() to notify end users about them.
Both ways are required to fit several kinds of reporting tools (runtime
collectors and static analyzers). This works well when calling a method or
when instantiating a class.

In other situations, annotations are not helpful, because the deprecations
map to e.g. a specific value of an argument. That's something that no
static analyzer can spot and only inline trigger_error() can report.

When extending a class, in debug mode only, we have a special autoloader
that uses reflection to check whether the parent class or one of its method
are deprecated. It triggers a deprecation notice when it detects this
situation. Before triggering the notice, it also checks the namespace of
the parent class: if it matches the namespace of the child class, no
deprecations are reported. This allows the very common pattern of ("new
API" extends "legacy API"), which is required to make the "new API" pass
the old type hints that we have to keep in place for BC. This aspect is
unaddressed in your proposal (and doesn't need to be actually, pure
declarations are fine, others can build or reuse the same logic in
userland).

We also have another strategy to report deprecations related to
inheritance, which is purely local (no need for the autoloader I just
described). By using reflection, we sometimes check inline whether the
currently called method is being overridden, and we trigger a deprecation
if yes (or e.g. if the signature of the child method misses an argument
that we want to add in the future).



> 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.
>

There is no cooperation ever between the error handler and the decision to
locally report a deprecation. All deprecation triggering sites in the code
decide on their own. Also, the backtrace is never ever used to make such a
decision.

In order to *not* trigger a deprecation when calling a deprecated method,
we call the method with an extra argument that we read via func_get_arg().
There is no other more accurate way.

The variety of strategies that we use is quite high because the variety of
situations that exists in real apps is also quite high, so we had to find a
way for each of them. I invite you to check the source code of Symfony,
branch 3.4 or 4.4, to have an example of each of them. I would be happy to
guide you (or anyone reading) through this if you want to take the journey,
that's quite useful for the discussion if we want to get deeper into it.



> 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.
>

That'd be a quite unsatisfactory outcome...



> As an additional step, I very much wish 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.
>

A free-form text is what we're using since years yes. It works, but it
makes it hard if not impossible to report structured info about which
package block and cannot be updated to their next major. Providing this
info in a structured way is useful and that'd what I'm trying to convey
here. I think we *can* assume that every deprecation will be able to report
some package+version info (it doesn't have to be a composer package, what
matters is having the fields easily identifiable)

Nicolas

>

--0000000000003e081505a5179470--