Attributes/Annotations Case Study: Drupal

Hi Larry,

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."

What I would like to say yes. Very very loudly. But I am not sure about
what is what or what defined what in your complex example

Do you have a link to the source file so I can make an informed reply
please?

Hi!

That you for bringing real usage examples. This is an interesting point
to consider.

Reading through it, however, I can't help but wonder why these things
are not just data? I mean, why they are kept in the comments? If there
are reasons like performance, there could be solutions developed, and
"keeping things together" doesn't exactly work when the things are
2-screen-long data array and I presume equally complex class - they are
two separate and independent entities anyway, and their synchronization
has to be explicitly managed anyway, nobody can ensure they are in sync
just by looking at it - in fact, given its size, nobody can anything
just by looking at it. It's not really human-readable anymore than
database dump is human-readable - the data is there, true, but it's not
exactly how you'd prefer to interact with it.

That said, of course I imagine there are reasons why Drupal developers
made these choices, and I don't think it makes much sense to argue about
them now. I just outlined my first impression, but if it sounds wrong
and uninformed, just ignore it :) It does, however, makes sense arguing
about whether it is something we have to support directly in the
language, and to which lengths we should go to do so.

defining a given extension. Saying "well just abandon your approach
entirely" is not a satisfying answer.

I would not say "abandon your approach" - what works for you works for
you, so trying to convince you otherwise would be wasting your time. I
would say that if somebody is developing a new system and are
considering this approach - I would advise checking a different
direction.

That is to say, I would be perfectly happy if PHP had annotations that
do not support such use case, at least not directly. I know that sounds
somewhat insensitive to Drupal developers, and that's not my intent do
dismiss their concerns, but I think we should not take it as a primary
requirement, sine qua non.

My opinion is good design should figure out the best use cases for the
feature and serve them as much as possible, without trying to overload
the system to serve every need and every use case there could be. Doing
80% of cases well is much better than doing 99.999% cases poorly. My
opinion is hundred-element data dumps are well outside of the 80% for
annotations. If whatever design we end up with supports it - great, if
it doesn't - that's fine too.

• @Block(
• id = "system_branding_block",
• admin_label = @Translation("Site branding")
• )

Why not do just:

@Block([ "id" => "system_branding_block", "admin_label" =>
Translation("Site branding")])

Or, in current RFC syntax:

<<Block([ "id" => "system_branding_block", "admin_label" =>
Translation("Site branding")])>>

Now, we get to a touchy issue of how arrays can be consumed by
annotations clients and what "Translation" is. Here we have two news, as
it should be, good and bad:

• Good news is that AST makes kind of easy to have Translation have any
  semantic the client wants
• Bad news is that currently RFC provides no tools for actually
  operating on deeper data structures, AFAIK. If such tools are provided,
  then I don't see why this won't work.

Also, of course, namespacing is a must, but that was already discussed
and I think being taken care of.

Yoinks. Now let's try and turn that into attributes. Here's my first

Here it's more complicated, because some data items clearly should be
different annotations, like "handlers" - these are probably something like:
<<StorageHandler(Drupal\node\NodeStorage)>>
<<FormHandler(Drupal\node\NodeForm)>>

etc. and some others may be data. Hard to know without understanding
true semantics of the thing.

OK, let's take that to its logical conclusion and directly map in the
full annotation:
<<ContentEntity([

That could work too, but see above about deep data, and also it looks
bad semantically - it's like having a class with one method five miles
long which accepts 50 parameters depending on what needs to be done. Not
enough structure. Again, this is saying without knowing semantics, so
maybe it's all wrong.

In any case, I think if we have: a) namespacing and b) tools to convert
AST to some useable data at least supporting arrays and maybe some kind
of namespace resolution - then I think it still can be supported, with
some effort of course but I would expect moving data of this complexity
to a different system to take some effort in any case.

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.)

True. If we had named parameters, we could probably avoid that, but we
don't have them. You could make each parameter separate annotation, but
that might look insane. Or maybe not, maybe just unusual.

You could also have a tool that just reads annotation and validates
them. Just like static analysis tools for PHP.

Without really knowing your semantics I don't see how else the system
would know route_provder is not a correct parameter. The only thing that
really can validate such things right now in PHP is interface, and if we
really stretch it, maybe a class - but I'm not sure how that would
really fit. And if we go that route, then we probably couldn't have the
freedom AST allows us.

1. ::class constants should resolve at compile time, relative to use
   statements, anywhere in the annotation.

That makes sense, but then it's unclear why should that be unique for
::class? Then all constants should work this way I think.

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:

That makes sense too. I think Dmitry said he plans to add that.

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

I am somewhat reluctant to define a new conceptual thing just for
annotations. Class seems natural at the first glance but implications of
it are unclear - what does it mean to instantiate such class? How
annotation data is now related to the object of this class?

Another option could be to (ab)use interfaces in a following fashion: if
we define

<<__Attribute>>
interface BlockPlugin() {
function id();
function admin_label();
}

Then when we say:

<<BlockPlugin(["id" => "system_branding_block", "admin_label" =>
Translation("Site branding") ])>>

we get an object that implements this interface and id() and
admin_label() returns something relevant. There are problems with this
approach:

1. Using array as parameter is ugly. Named arguments is what we need but
   d'oh. Also enshrining array-as-parameters in syntax which is worse.

2. It is not clear what exactly admin_label() should return and how
   Translation() should work - is it AST? Is it some kind of object? Should
   we just give up on Translation and do it in some other way like this?

<<__Attribute>>
interface BlockPlugin() {
function id();
<<Translatable>>
function admin_label();
}

Note that this is a way to enable nesting of attributes without getting
the data part too complex. But of course then all admin_labels must be
translatable or not-translatable, not half-way.

So the idea is half-baked but maybe it can be used, think about it.

Stas Malyshev
smalyshev@gmail.com

On Sat, Apr 30, 2016 at 9:47 AM, Larry Garfield larry@garfieldtech.com
wrote:

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.

I would love it if annotations were just classes with public properties,
and the annotation shorthand for instantiating and setting properties was
generally avaliable. At the moment if you wish to instantiate an object and
set some public properties, you have to use a temporary variable

$album = new Album();

$album->name = "Albumius";
$album->artist = "Artistus";

$album->year = 2013;

return $album;

or define setters for all the properties so you can chain them

return (new Album())

->setName("Albumius")

->setArtist("Artistus")

->setYear(2013);

or forego the benefits of classes and use an array

return [
'name' => "Albumius",
'artist' => "Artistus",
'year' => 2013,
];

C# has an "instantiate-and-set-properties" shorthand like this, which would
be great to have in PHP:

return new Album() {
name = "Albumius",
artist = "Artistus",
year = 2013,
};

and if you drop the "new" (and constructor parens are already optional)
you've got a pretty good annotation syntax right there.

<<Links {
canonical = "/node/{node}",
deleteForm" = "/node/{node}/delete",
editForm" = "/node/{node}/edit",
versionHistory" = "/node/{node}/revisions",
revision" = "/node/{node}/revisions/{node_revision}/view",
}>>

<<Foo("param", "param")>>

<<Blah(5) { foo = 5 }>>

So the annotation syntax could just be class instantiation without the
"new".

(maybe there should be a $ before the property names, not sure)

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