Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:93079
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain zend.com designates 157.56.110.135 as permitted sender)
To: Larry Garfield <larry@garfieldtech.com>, 'PHP internals'
	<internals@lists.php.net>
References: <5723F2AE.2020806@garfieldtech.com>
Message-ID: <d158ceff-b888-604c-387c-942239c0e783@zend.com>
Date: Thu, 5 May 2016 09:35:01 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.0
MIME-Version: 1.0
In-Reply-To: <5723F2AE.2020806@garfieldtech.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit
SpamDiagnosticOutput: 1:23
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 May 2016 06:35:15.3742 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLUPR0201MB1777
Subject: Re: [PHP-DEV] Attributes/Annotations Case Study: Drupal
From: dmitry@zend.com (Dmitry Stogov)



On 04/30/2016 02:47 AM, Larry Garfield wrote:
> Most of the examples that have been given so far are either trivial 
> boolean flags or data validation rules to be evaled.  In practice, 
> very little of Drupal's use of annotations in Drupal 8 fit either 
> category.  Rather, they're used primarily as, in essence, a serialized 
> metadata object describing a class, which is used for registering that 
> class and potentially others.  I figured I'd give the proposed syntax 
> a try with some Drupal examples and see how well it fit.
>
> Disclaimer: I'm sure someone will pipe up with "your use case is 
> invalid because you shouldn't be using annotations that way."  I will 
> be the first to agree that Drupal loves to take everything it does to 
> an extreme, and some things may be better done other ways. However, 
> these are still real-world use cases (currently built with Doctrine 
> Annotations) that people are going to want to try and reimplement 
> eventually using a core language feature.  This much data is put in 
> one place primarily for DX reasons, to give developers a one-stop-shop 
> for defining a given extension.  Saying "well just abandon your 
> approach entirely" is not a satisfying answer.
>
> Summary: It doesn't fit well at all, and there's features missing that 
> would prevent Drupal from being able to use the proposed attributes 
> RFC as-is, even without talking about classes-as-annotations.  A 
> series of improvement request/suggestions are listed at the end of 
> this email.
>
> Simple example:
>
> Drupal plugins (usually) use annotations.  Here's a simple example:
>
> /**
>  * Provides a block to display 'Site branding' elements.
>  *
>  * @Block(
>  *   id = "system_branding_block",
>  *   admin_label = @Translation("Site branding")
>  * )
>  */
> class SystemBrandingBlock {
>
> }
>
> This defines a "block" (type of plugin).  It's unique machine name 
> identifier is "system_branding_block", and its human-facing label is 
> "Site branding", which is marked as a translatable string. That all 
> seems reasonable to include here.
>
> Here's what I came up with for a possible attributes version:
>
> <<PluginType('Block')>>
> <<PluginId('system_branding_block')>>
> <<PluginAdminLabel('Site branding')>>
> class SystemBrandingBlock {
>
> }
>
> Not too bad at first blush, but there's 2 problems.
It's also possible to write:

<<Drupal(@Block([
       "id" = "system_branding_block",
       "admin_label" = @Translation("Site branding")
]))>>

Then you'll need you own layer that translates "Drupal" attributes from 
AST to everything you like.

>
> 1) There's no indication that the label is a translatable string. One 
> could hard-code that logic into whatever processing happens for 
> PluginAdminLabel, but then there's no indication for our gettext 
> scanner that "Site branding" is translatable and should be extracted 
> for translation.
>
> 2) If we want to say that the value "Block" corresponds to a class 
> (something that would be up to the parser to do), there's no 
> indication of the namespace against which to resolve "Block".  The 
> alternative would be to require including the full class name string, 
> like so:
>
> <<PluginType('Drupal\Core\Block\Annotation\Block')>>
>
> But that DX is quite terrible.  We introduced ::class in 5.5 for a 
> reason.  Better would be:
>
> <<PluginType(Block::class)>>
>
> But that works only if the attribute parser resolves Block::class 
> against the currently "use"d namespaces so that it's a full class name 
> string when reflection picks it up.  If not, then that means the 
> user-space parser needs to catch that, then go back to the file and 
> figure out the available use statements and resolve against those.  
> It's doable, but ugly and certainly more work than I'd want to put in 
> as someone writing such a parser.
>
> I don't know if that's a feature of the patch at the moment, but it 
> would need to be.
>
> So even in a simple case we have insufficient functionality.
>
> Complex example:
>
> OK, let's go to the other end and look at an example that is way more 
> complicated.  (Yes, maybe too complicated.)  Doctrine annotations are 
> also used to define Entity Types, which correspond to a class.  Here's 
> the annotation for a Node, in all its glory:
>
> /**
>  * Defines the node entity class.
>  *
>  * @ContentEntityType(
>  *   id = "node",
>  *   label = @Translation("Content"),
>  *   bundle_label = @Translation("Content type"),
>  *   handlers = {
>  *     "storage" = "Drupal\node\NodeStorage",
>  *     "storage_schema" = "Drupal\node\NodeStorageSchema",
>  *     "view_builder" = "Drupal\node\NodeViewBuilder",
>  *     "access" = "Drupal\node\NodeAccessControlHandler",
>  *     "views_data" = "Drupal\node\NodeViewsData",
>  *     "form" = {
>  *       "default" = "Drupal\node\NodeForm",
>  *       "delete" = "Drupal\node\Form\NodeDeleteForm",
>  *       "edit" = "Drupal\node\NodeForm"
>  *     },
>  *     "route_provider" = {
>  *       "html" = "Drupal\node\Entity\NodeRouteProvider",
>  *     },
>  *     "list_builder" = "Drupal\node\NodeListBuilder",
>  *     "translation" = "Drupal\node\NodeTranslationHandler"
>  *   },
>  *   base_table = "node",
>  *   data_table = "node_field_data",
>  *   revision_table = "node_revision",
>  *   revision_data_table = "node_field_revision",
>  *   translatable = TRUE,
>  *   list_cache_contexts = { "user.node_grants:view" },
>  *   entity_keys = {
>  *     "id" = "nid",
>  *     "revision" = "vid",
>  *     "bundle" = "type",
>  *     "label" = "title",
>  *     "langcode" = "langcode",
>  *     "uuid" = "uuid",
>  *     "status" = "status",
>  *     "uid" = "uid",
>  *   },
>  *   bundle_entity_type = "node_type",
>  *   field_ui_base_route = "entity.node_type.edit_form",
>  *   common_reference_target = TRUE,
>  *   permission_granularity = "bundle",
>  *   links = {
>  *     "canonical" = "/node/{node}",
>  *     "delete-form" = "/node/{node}/delete",
>  *     "edit-form" = "/node/{node}/edit",
>  *     "version-history" = "/node/{node}/revisions",
>  *     "revision" = "/node/{node}/revisions/{node_revision}/view",
>  *   }
>  * )
>  */
> class Node {
>
> }
>
> Yoinks.  Now let's try and turn that into attributes.  Here's my first 
> attempt:
>
> <<PluginType('ContentEntity')>>
> <<PluginId('node')>>
> <<ContentEntity\Label('Content')>>
> <<ContentEntity\BundleLabel('Content type')>>
> <<ContentEntity\Handler\Storage(NodeStorage::class)>>
> <<ContentEntity\Handler\StorageSchema(NodeStorageSchema::class)>>
> <<ContentEntity\Handler\ViewBuilder(NodeViewBuilder::class)>>
> <<ContentEntity\Handler\Access(NodeAccessControlHandler::class)>>
> <<ContentEntity\Handler\ViewsData(NodeViewsData::class)>>
> <<ContentEntity\Handler\Form\Default(NodeForm::class)>>
> <<ContentEntity\Handler\Form\Delete(NodeDeleteForm::class)>>
> <<ContentEntity\Handler\Form\Edit(NodeForm::class)>>
> <<ContentEntity\RouteProvider\Html(NodeRouteProvider::class)>>
> // ...
> class Node {
>
> }
>

you don't need to split your annotation into many attributes. You should 
just adopt its syntax to become a valid PHP expression.
This expression is not going to be evaluated. It's going to be just 
parsed into AST. and then you may traverse this AST and transform it 
into other data structures.
The key idea of RFC was not to invite another language for meta-data, 
but use PHP language itself.


> I gave up at that point, as I'd already identified several problems.
>
> 1) The attribute names themselves are horribly long, because they're 
> not namespaced at all.  That means I have to namespace them myself in 
> the annotation, which leads to grotesquely long tokens.
>
> 2) Again, I need to list the name of a class in the annotation. That 
> means either a stupid-long-string (which therefore means I can't catch 
> stupid typos with static analysis) or resolving ::class constants.  I 
> went with the latter here because the alternative is basically unusable.
>
> 3) Nested data is unsupported, except through manual namespacing.
>
> 4) Hashes are totally unsupported. 
> <<ContentEntity\Handler\Form\Default(NodeForm::class)>> is a terrible 
> idea, as even with my own parser I now cannot tell which part of that 
> string is supposed to be a class name to map the data to and which isn't.
>
> 5) Because the attribute names are just strings, I am repeating very 
> long values with no static analysis support at all.  One typo in that 
> string, even though it's not quoted like a string, means stuff will 
> just not work and I don't know how to catch it other than visual 
> inspection (aka, "hey developer, guess!").
>
> OK, let's try using the AST format.  That gets a little bit better:
>
> <<PluginType('ContentEntity')>>
> <<PluginId('node')>>
> <<ContentEntity\Label('Content')>>
> <<ContentEntity\BundleLabel('Content type')>>
> <<ContentEntity\Handler\Storage(NodeStorage::class)>>
> <<ContentEntity\Handler\StorageSchema(NodeStorageSchema::class)>>
> <<ContentEntity\Handler\ViewBuilder(NodeViewBuilder::class)>>
> <<ContentEntity\Handler\Access(NodeAccessControlHandler::class)>>
> <<ContentEntity\Handler\ViewsData(NodeViewsData::class)>>
> <<ContentEntity\Handler\Form(['default' => NodeForm::class,
>                               'delete' => NodeDeleteForm::class
>                               'edit' => NodeForm::class]>>
> <<ContentEntity\RouteProvider(['html' => NodeRouteProvider::class]>>
> class Node {
>
> }
>
> At least now I can define the hash in a parsable fashion. However, 
> it's not clear to me if I can catch errors there with static 
> analysis.  Note, for instance, that I omitted the comma on the 
> 'delete' line.  That would be a parse error in normal code. Would it 
> be a parse error on compile?  I would hope so, since it would be a 
> parse error when I tried to reflect the attributes.  It doesn't 
> address the other issues, though.

yes. it's going to be a parse error at compile time.
The patch was provided and it's better to try it.
>
> OK, let's take that to its logical conclusion and directly map in the 
> full annotation:
>
> <<ContentEntity([
>   'id' => 'node',
>   'label' => 'Content',
>   'bundle_label' => 'Content type',
>   'handlers' => [
>     'storage' => NodeStorage::class,
>     'storage_schema' => NodeStorageSchema::class,
>     'view_builder' => NodeViewBuilder::class,
>     'access' => NodeAccessControllerHandler::class,
>     'views_data' => NodeViewsData::class,
>     'form' => [
>       'default' => NodeForm::class,
>       'delete' => NodeDeleteForm::class,
>       'edit' => NodeForm::class,
>     ],
>     'route_provder' => [
>       'html' => NodeRouteProvider::class,
>     ],
>   ],
>   // ...
> ])>>
> <<ContentEntity\Translatable>>
> <<Links([
>   "canonical" => "/node/{node}",
>   "delete-form" => "/node/{node}/delete",
>   "edit-form" => "/node/{node}/edit",
>   "version-history" => "/node/{node}/revisions",
>   "revision" => "/node/{node}/revisions/{node_revision}/view",
> ])>>
> class Node {
>
> }
>
> I broke it up a little bit, but that may or may not make sense in 
> practice.
>
> What do we get with this approach?  Well, we can model the full data 
> structure.  That's good.  If ContentEntity is supposed to map to a 
> class in my system it would be slightly less fugly to use a full class 
> name there now, as there's only maybe 3 attributes instead of 30.  
> Still, we're really only getting any benefit out of it if:
>
> 1) The ::class constant acutally works as I've shown here.
>
> 2) The parser understands that I'm trying to define an array structure 
> and can catch syntax typos for me.
>
> Otherwise, there's really no benefit over sticking the string in the 
> docblock as we do now.
>
> Additionally, even then it's still "just an array", by which I mean an 
> anonymous struct, by which I mean "you get no help at all on typos."  
> You may have noticed that I typoed "route_provder" instead of 
> "route_provider" above.  An an array key, that is a silent runtime 
> error that's crazy hard to debug.  Forcing any meaningful data 
> structure into anonymous structs is doing everyone a disservice.  (As 
> a Drupal developer, I can attest to that fact with personal scars.)
>
> Recommended changes:
>
> 1) ::class constants should resolve at compile time, relative to use 
> statements, anywhere in the annotation.

we don't have fully constructed classes at compile time. Classes may be 
used during transformation from plain arrays and AST into application 
specific data structures.
>
> 2) Namespacing for attribute names.  This is necessary for both 
> collision-avoidance and to minimize typing of obscenely long attribute 
> names.  The easiest way to do that would be:
>
> 3) Some way to provide a data definition for the annotation that can 
> be checked at compile time.  This could be classes, a la Doctrine. It 
> could be a new Annotation type as others have suggested.  It could be 
> something else, akin to a struct like Liz Smith keeps toying with.  
> I'm flexible here.  But some way to provide a data definition for the 
> annotation that is more robust than "you get an AST for an associative 
> array that you can eval to an array yourself, good luck" is, I 
> believe, mandatory for these to be successful for more than the most 
> trivial use cases.  If that data definition is a class or class-like 
> type, that also resolves the namespace question very easily.
>
> Note: We do NOT need objects created at compile time or include time, 
> or even instantiation of the annotated class time.  Only at 
> reflection-lookup time.  All that's needed before that is syntax 
> validation (both compiler and IDE, once IDEs catch on).  If that can 
> go as far as key validation at compile time, great.  If that can only 
> happen at reflection-lookup time, that's acceptable as long as it 
> happens.

This is the way patch works. It performs only syntax validation at 
compile time (no key validation). And at reflection time you may 
validate and translate base structures into whatever you like.
>
> 4) I don't know if there's a reasonable way to "nest" annotations. I 
> do not have a solution for the translatable string marker issue, but 
> it is a feature of Doctrine that Drupal leverages heavily and we would 
> be loathe to lose.  (It may even be a deal breaker.)

@ is used as silence operator, but it's still a valid PHP expression syntax

<<name(@Translate("..."))>>
>
> I hope this case study has been useful.  To reiterate, I am in favor 
> of PHP adding annotation/attribute/whatever-you-call-it support; I 
> just want to make sure it is robust enough to support the use cases 
> that I know exist in the wild.

yeah. thanks for spending your time. I hope my answers would help you 
adopting attributes idea for your needs.

Thanks. Dmitry.

>
> --Larry Garfield
>