Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:109711
MIME-Version: 1.0
References: <CAEPJdunSr8PkBtSLO8g_JMLzM4cFVnXsQSEMFmu2RN25XT=grw@mail.gmail.com>
 <CAEPJdumeStFzWRcEtU3NjEgmxZ_08wbfyJezhHOEzZbzdNWC=w@mail.gmail.com>
 <CANUQDChrRXg1Jk3ig20bBTNKg8QbZZY=JH+9GDY4e_Rtyz4kEA@mail.gmail.com> <CAEPJdun+MFYo9WXum02pP0mxTYZ5YaL_4qYbZoTA-18iXTjepQ@mail.gmail.com>
In-Reply-To: <CAEPJdun+MFYo9WXum02pP0mxTYZ5YaL_4qYbZoTA-18iXTjepQ@mail.gmail.com>
Date: Mon, 20 Apr 2020 08:47:45 +0200
Message-ID: <CANUQDCgjXP8D7XtCQgx33MOwCicnNqOVgYOkm4CHP-Up6Ft1gA@mail.gmail.com>
To: Benjamin Eberlei <kontakt@beberlei.de>
Cc: PHP Internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Subject: Re: [PHP-DEV] Re: [RFC] Attributes v2
From: me@kelunik.com (Niklas Keller)

Hey Benjamin,

thanks for your answers!

> <<Attribute>> would best require a namespace (PHP\Attributes) as I feel
> claminig "Attribute" class in the main namespace might cause problems for
> users.

While this is true, I don't see how Attribute is different to any
classes introduced in the "recent" versions like \Generator or \Error.
Maybe Nikita can run his analyzer to see how widespread its usage is?

> But there is no PHP namespace yet and proposals about this have just
> gotten to the list. I have therefore looked to PhpToken from nikitas recent
> RFC as an example for naming, because several contributors mentioned that
> the PHP\Attributes namespace I suggested in an earlier version of the RFC
> would be an instant "no" vote, because of the lack of previous decision on
> this topic.

Some prefix is pretty similar to a namespace, and I don't like a new
naming convention being part of this RFC. For PhpToken the naming
might make sense, as "Token" alone might be seen as a too generic
term.

> The reason these are plain PHP objects and not some special case is that
> you can re-use them for configuration from different sources. Symfony
> Validator is a good example where the "attributes" could map to the
> validator configuration classes that are also used from XML, YAML, ...

Sounds fine!

> Yes, INSTANCE_OF attempts to load each attributes class, but if an
> attribute class cannot be looked up (not autoloadable) it gets skipped
> without error (subject to error handling of autoloader implementation, but
> for Composer it skips).

This seems rather strange to me, but I can definitely see the value if
you're using no-dev deployments, for example. I think I'd be better to
disallow inheritance for attributes and skip the autoloading and
INSTANCE_OF parameter here.

> >
> > I expect annotations to be readonly, which classes as outlined in the
> > RFC cannot currently enforce in PHP. A syntax similar to interfaces
> > might be appropriate, I'm not sure whether that has been discussed:
> >
>
> The write-once / readonly RFC was rejected and only internal classes can
> implement this behavior (see ext/dom). But userland attributes also map to
> userland classes, so as you say this is not possible. However given this
> RFC maps to existing concepts.

I'd be possible if the actual objects are created in core and userland
only provides the contracts (interfaces) for these attributes.

> I don't see how preventing instantiation or requiring readonly in userland
> produces any benefits over this described use-case in the RFC. This would
> make attributes much stricter than everything else in the language, I don't
> see how its fair to impose this on a new feature like Attributes, when the
> language doesn't even have concepts for this strictness for regular classes
> (containing more important code). Mapping to "normal" classes is the way C#
> implements attributes, I don't think this should be more complex than that.

Readonly is just something I'd like to see, it's not a requirement and
I'll still vote yes if the more important points are solved.

What's more important here IMO is the restriction of inheritance,
mainly because the raw arguments are exposed via reflection and won't
be compatible between parent and child attribute in any case.

There are a few options I see to solve this:

 - Require constructor compatibility for attribute classes (only other classes)
 - Forbid extending attribute classes entirely
 - Remove getArguments() from reflection

I think my preferred solution would be to forbid inheritance, because
that also solves the autoloading inconsistencies (given that
implemented interfaces aren't respected in
`ReflectionFunction::getAttributes()` and others.

> Extensions, and Third Party library authors can easily guard their
> attribute classes against writes from the outside with the usual OOP
> abstractions and if application authors deem their attributes to be
> writable that is their business.

That's probably fine, yes, if `getAsObject()` / `toObject()` /
`createObject()` always returns a new object.

> Named parameters might some day come to PHP in the future, and this is
> precisely the argument for treating an attribute like a regular php class
> with a constructor, because the named params syntax would look exactly the
> same in attribute declarations then, increasing language consistency.
>
> The reason I went with the C# way of mapping to a "plain class" is
> simplicity. The concept of attributes is already not the easiest nut to
> crack, inventing another keyword and a structure that looks like a class,
> but has very different implications requires a lot of words in the
> documentation and doesn't provide the easiest access to beginners.

I guess writing your own annotations and processing them isn't a
beginners topic, but using them to attribute code definitely is.

> >
> > Finally, the naming of "getAsObject" should IMO be improved,
> > especially if I read the RFC correctly and autoloading and constructor
> > invocation is performed by this method. I think "createObject" or
> > "toObject" might be better names.
> >
>
> The name getAsObject is an implementation detail in my perspective. I am
> open for a better name.

Do you know whether we already have something similar in core?

> > In summary, I'd probably vote "no" on the current proposal, even if
> > it's one of the most missed features, because I think we can do
> > better, and there's only one chance.
> >
>
> Sorry to hear and I hope you reconsider after reading my reply, as I built
> this RFC around extensibility in the future primarily to address a lot of
> voter feedback from the last RFCs.
>
> For example things you mentioned: named parameters, readonly properties,
> constructor property promotion, PHP namespace. These topics are all
> currently or recently discussed and would all automatically lead to
> improvements for attributes, when they "just" map to classes and their
> constructor. If we would go with special cases for Attributes, we have to
> solve all of these problems a second time, and this RFC would explode in
> complexity.

I think it's fine solving these in the future. However, the naming and
autoloading behavior can't be solved in the future, so I hope we can
get some improvements in before the vote.

Best,
Niklas