Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:108930 Return-Path: Delivered-To: mailing list internals@lists.php.net Received: (qmail 6217 invoked from network); 10 Mar 2020 00:44:19 -0000 Received: from unknown (HELO php-smtp4.php.net) (45.112.84.5) by pb1.pair.com with SMTP; 10 Mar 2020 00:44:19 -0000 Received: from php-smtp4.php.net (localhost [127.0.0.1]) by php-smtp4.php.net (Postfix) with ESMTP id 337651801FD for ; Mon, 9 Mar 2020 16:05:05 -0700 (PDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on php-smtp4.php.net X-Spam-Level: X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,RCVD_IN_DNSWL_LOW,SPF_HELO_PASS,SPF_NONE autolearn=no autolearn_force=no version=3.4.2 X-Spam-ASN: AS11403 64.147.123.0/24 X-Spam-Virus: No X-Envelope-From: Received: from wout4-smtp.messagingengine.com (wout4-smtp.messagingengine.com [64.147.123.20]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by php-smtp4.php.net (Postfix) with ESMTPS for ; Mon, 9 Mar 2020 16:05:04 -0700 (PDT) Received: from compute7.internal (compute7.nyi.internal [10.202.2.47]) by mailout.west.internal (Postfix) with ESMTP id 9B5CC3FA for ; Mon, 9 Mar 2020 19:05:02 -0400 (EDT) Received: from imap26 ([10.202.2.76]) by compute7.internal (MEProxy); Mon, 09 Mar 2020 19:05:02 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm2; bh=vn+Tka xgBZZn+wGPOiY8rT4ynO4QplYLN7PZqTcQi2w=; b=3l9wxkWzFiWBhy0b8tHhIH 1OxO0HNfYZsAIpy66TmKTqe+zGzk7cgFabgVtA8I+tWiaGBjBhEiZRkjd7wLuZzB 0QdtMhnFvVPQADx06KW60q+djk2fZGv1P1wZ4UdjcdbeDaB7hBDV7xjWBtslfQHA wzS1wh+IstvsNs6V3sdQeKDC4eI07Sd3vPqznBAWpzKnF6b5rTHJ/nU4j0P8Tf/z Vb+fj2RO4aTmbkrmNG657mK+8Vwgi9jsT2Ko5jdGxnG9cwUjcTh+xU7r2XXsT8hX TS17QAWa6WKnZrQ5XbmFSHlSTm7KY3OkgSBhA4qleo7h/ewTE4fbZ+FDZdnWtMxg == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedugedrudduledgtdegucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepofgfggfkjghffffhvffutgesthdtredtreertdenucfhrhhomhepfdfnrghr rhihucfirghrfhhivghlugdfuceolhgrrhhrhiesghgrrhhfihgvlhguthgvtghhrdgtoh hmqeenucffohhmrghinhepphhhphdrnhgvthdpghhithhhuhgsrdgtohhmnecuvehluhhs thgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomheplhgrrhhrhiesghgrrh hfihgvlhguthgvtghhrdgtohhm X-ME-Proxy: Received: by mailuser.nyi.internal (Postfix, from userid 501) id CB9F214200A2; Mon, 9 Mar 2020 19:05:01 -0400 (EDT) X-Mailer: MessagingEngine.com Webmail Interface User-Agent: Cyrus-JMAP/3.1.7-991-g5a577d3-fmstable-20200305v3 Mime-Version: 1.0 Message-ID: <042cbb74-cd20-4762-a7d4-d6614a6c08a7@www.fastmail.com> In-Reply-To: <2461944e-bc66-4941-aef4-ed07aefa3858@www.fastmail.com> References: <2461944e-bc66-4941-aef4-ed07aefa3858@www.fastmail.com> Date: Mon, 09 Mar 2020 18:04:41 -0500 To: "php internals" Content-Type: text/plain Subject: Re: [PHP-DEV] [RFC] Attributes v2 From: larry@garfieldtech.com ("Larry Garfield") On Mon, Mar 9, 2020, at 5:02 PM, Larry Garfield wrote: > 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: > > <> > 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): > > <> > 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\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; > } > } > > <> > 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. Just for kicks, I decided to see what attributes would look like in Tukio to figure out what using the API would look like. So here's a few possibilities as a test case. The original code is here: https://github.com/Crell/Tukio/blob/master/src/ProviderBuilder.php#L69 It's implementing basically the same idea as Symfony's EventSubscriberInterface, for those familiar with it. Only right now you can't set anything directly and have to use a separate callback to have non-default configuration. The intent here is to replace that separate callback with attributes. Code here: https://gist.github.com/Crell/22fd255efe389f2eaf01fc89ab1d85d9 In the single attribute version, it works pretty well but it's annoying that the various options have to be in order, and are not self-documenting. I left out the option of before/after ordering instead of priority. I'm not sure how I'd do it there other than making it 3 different annotation classes, but then I'd need to add code to enforce that you use only one of the three. In the multi-attribute version, it's decent amount more code and it has a type switch in the middle. Interestingly I did it a different way than the RFC's example. I first pulled out the attribute objects en masse, then did an if-elseif dance. The RFC example had a switch on the type property of the reflection object, and then pulled out the actual attribute object itself within each switch case. Neither one seems particularly nice to me. Finally I did a single attribute version that takes an array parameter to make it more self-documenting. That made it much easier to handle internally, and I was able to add the priority/before/after logic that would have been too annoying to do in the other examples. Overall thoughts: The reflection API is annoying, but that doesn't come from annotations. I'd love to be able to clean up those RTTI statements. Neither the one here nor the one in the RFC now is particularly pleasant. That's possibly out of scope (we are talking about an array of multiple objects of unknown concrete type, so RTTI is probably unavoidable), but I wonder if having Attributes have a base class with some built in functionality (eg, the pieces of the reflection object itself) would be useful? Essentially combining the reflection object and the attribute object so we can skip a step. Just brainstorming here. Visually as a user of the attribute, I somewhat prefer the multi-attribute version. Not completely, though. I'm still torn. The array param version seems the most flexible, but also has a lot of sigil-soup going on. And while it does give me a place to do some extra error handling, it has all the same problems that $options parameters always have. Hopefully this test case is useful and educational. I'd still love to see this get in. (If some of the examples aren't quite clear, let me know.) --Larry Garfield