Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:108929
User-Agent: Cyrus-JMAP/3.1.7-991-g5a577d3-fmstable-20200305v3
Mime-Version: 1.0
Message-ID: <2461944e-bc66-4941-aef4-ed07aefa3858@www.fastmail.com>
In-Reply-To: 
 <CAEPJdunSr8PkBtSLO8g_JMLzM4cFVnXsQSEMFmu2RN25XT=grw@mail.gmail.com>
References: 
 <CAEPJdunSr8PkBtSLO8g_JMLzM4cFVnXsQSEMFmu2RN25XT=grw@mail.gmail.com>
Date: Mon, 09 Mar 2020 17:02:51 -0500
To: "php internals" <internals@lists.php.net>
Content-Type: text/plain
Subject: Re: [PHP-DEV] [RFC] Attributes v2
From: larry@garfieldtech.com ("Larry Garfield")

On Mon, Mar 9, 2020, at 9:42 AM, Benjamin Eberlei wrote:
> Hi all,
> 
> I want to resurrect Dmitrys Attributes RFC that was rejected for 7.1 in
> 2016 with a few changes, incorporating feedback from the mailing list back
> then and from talking to previous no voters.
> 
> The RFC is at https://wiki.php.net/rfc/attributes_v2
> 
> A working patch is at https://github.com/beberlei/php-src/pull/2 though
> work around the details is still necessary.
> 
> The RFC contains a section with common criticism and objections to
> attributes, and I hope to have collected and responded to a good amount
> already from previous discussions.
> 
> There is also a fair amount of implementation detail still up for debate,
> which is noted in "Open Issues". I have pre-committed to one approach, but
> listed alternatives there. On these issues I am looking for your feedback.
> 
> greetings
> Benjamin

I am very excited to see this back, and overall I like the approach!

Thoughts in no special order:

* It's not entirely clear, but the values in an argument-carrying attribute are passed to its constructor variadically?  Viz:

<<Foo(1, 2, 3)>>
function stuff() {}

class Foo implements Attribute {
  public function __construct(int $first, int $second, int $third) { ... }
}

Yes?  (And then if you wanted to capture them all you could use a ...$args parameter in the constructor.)

* It looks like the only way to support named parameters would be to pass an associative array and decompose it yourself.  That's... better than nothing I guess but also we're back to the same lack of self-documentation we always lament.  Is there any opportunity to do named parameters in some other way?

* Is it possible to inline a sub-object, or no?  Viz, is this legal, or would we want it to be legal (I'm assuming it's not at the moment):

<<Foo('a', 'b', Bar('baz'))>>
function stuff() {}

* I *really* dig how you can filter annotations to just those that match a certain parent class/interface.  That was one of the nicer parts of PSR-14 so I'm really glad to see the same "make the type system earn its keep" approach here.  It encourages packages to interface-tag their annotations so they can easily grab "just theirs".  +1

* The "more complex Doctrine ORM use-case" example includes inlining multiple annotations together: <<ORM\Id, ORM\Column, ORM\GeneratedValue>> .  Is that just an oops since that possibility was only just added to the Open Issues?


Regarding open issues:

* If constant expressions are easy enough to support, I don't see a reason not to support them.

* I'm torn on the marker interface.  On the one hand, a marker interface would make it easier to audit a package for all of its attributes; just grep for "implements Attribute".  I'm sad that we didn't get a marker interface in PSR-14 for that reason.  On the other, if it doesn't do anything except be a marker then it doesn't really offer anything else; and it also potentially makes it harder to add some special casing of attributes in the future, say to offer "if you want this extra attribute behavior, use this interface/base class/whatever."  I could go either way here.

* Letting attributes say where they're legal would be a good case for such a more-than-marker interface.

* The aforementioned example of specifying where an attribute is legal implies that attribute classes can themselves have attributes.  True?  If so, that should be made explicit elsewhere in the RFC.  I'm not against attribute-ception, but that should be explicit.



I think my only significant pushback at the moment is that attributes here suffer from the same redundancy problem as any other value object.  To wit:

class Owner implements Attribute {

  public readonly string $name;

  public readonly string $title;

  public readonly int $age;

  public function __construct(string $name, string $title, int $age) {
    $this->name = $name;
    $this->title = $title;
    $this->age = $age;
  } 
}

<<Owner('Larry', 'Manager', 99)>>
function stuff() {}

That necessitates the same quadrupal-repetition that other constructors have.  Since I anticipate the 99% use case to be exactly that, the pain of that redundancy (and the mandatory unnamed order) will be felt rather dramatically here and thus hurts ergonomics.  Is there no way that we address that?

I realize the answer may be "no more than any other class, they all suck equally".  Are we at least certain then that if we can solve that in the future for classes generally that it will automatically work here?  (Eg, if new Owner({name: 'Larry', 'age' => 99, 'title' => 'Manager'}); became a legal shorthand for the above, it would automatically work for annotations as well.)

Overall nice work, and I hope to get to use it.


--Larry Garfield