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