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.
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 {
}
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.
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.
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.
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.)
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.
--Larry Garfield
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
