This is part of #1966246: [meta] Introduce specific annotations for each plugin type.

Now, when someone opens an entity type, the first thing they see is @EntityType, not @Plugin. Other than the namespace (which should be changeable), they don't need to know that entity types use plugin discovery.

And, now the EntityType class is the perfect place to document... itself.

Files: 
CommentFileSizeAuthor
#30 entitytype-1967294-30.patch56.53 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 54,359 pass(es).
[ View ]
#30 interdiff.txt4.69 KBtim.plunkett
#26 interdiff.txt675 bytestim.plunkett
#26 entitytype-1967294-26.patch54.72 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 54,451 pass(es).
[ View ]
#23 entitytype-1967294-23.patch54.72 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 54,469 pass(es), 10 fail(s), and 0 exception(s).
[ View ]
#23 interdiff.txt1.39 KBtim.plunkett
#20 entity-type-1967294-20.patch55.17 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 53,976 pass(es), 474 fail(s), and 58 exception(s).
[ View ]
#18 entity-type-1967294-18.patch55.13 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/lib/Drupal/Component/Annotation/Plugin.php.
[ View ]
#18 interdiff.txt2.48 KBtim.plunkett
#14 entitytype-1967294-8.patch55.82 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 54,443 pass(es).
[ View ]
#1 entitytype-1967294-1.patch63.6 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 54,210 pass(es).
[ View ]

Comments

Status:Active» Needs review
Issue tags:+Plugin system
StatusFileSize
new63.6 KB
PASSED: [[SimpleTest]]: [MySQL] 54,210 pass(es).
[ View ]

It also lets us remove the need for @Translation, at least for known keys. Unknown and tacked-on keys can still use it.

Yes! This looks great.

So, instead of

/**
* @Plugin(
*   id = "node",
*   label = @Translation("Node")
* )
*/

we will have

/**
* @EntityType(
*   id = "node",
*   label = "Node"
* )
*/

Does this hard-code what properties the annotation supports, and can no longer be extended by contrib?

Nope, it doesn't prevent contrib from doing anything.

It just hardcodes what it will automatically wrap in t(), so you don't have to use @Translate. Anything in contrib you add that you want wrapped in t(), you have to do @Translate("My string") yourself.

In fact, a good deal of these properties are tacked on by entity_translation, and shouldn't be on there.
We should have ET serve as the model for how one adds their own properties to an EntityType.

Well this seems like an entirely sensible idea.

I'm never going to disagree with more explictness in what you mean, and I *hate* @Translation from the very pit of my soul. This also implies we could actually document what the hell properties an @EntityType has, which was one of quicksketch's main concerns when he first hit annotations.

The advantage of @Translation is that you can scan actually the list of available strings.

Why do you care, if magical fairy elves take care of it automatically for you? :)

Damn. I knew it was too magical to be true.

+        $values[$key] = t($values[$key]);

Channeling Gábor from #1849610-36: Improve "add" links accessibility:

Problem is your approach with $whatever = 'boo'; t($whatever) makes 'boo' translatable only when this code runs. When the code is not run, there is *no way* to tell what is in $whatever when t($whatever) is looked at. Reality is modules cannot be *run* to find their translatable strings when localize.drupal.org collects translatable strings. ... So while $whatever = 'boo'; t($whatever) makes 'boo' translatable *on the site*, there is no community pre-translation possible. The UI can only be translated *after the fact*, *only on the site itself* once at least one user faced the untranslated UI at least once (so the Drupal runtime figures out the string to translate dynamically and puts it into the database).

I don't think we can kill @Translation, sadly. :\ Though why it is @Translation and not @t is beyond me (and not for this issue).

Um, well

class Translation implements AnnotationInterface {
  public function __construct($values) {
    $string = $values['value'];
    $options = array();
    if (!empty($values['context'])) {
      $options = array(
        'context' => $values['context'],
      );
    }
    $this->translation = t($string, array(), $options);
  }
}

How is that better?

#1933984: Support for Drupal 8 @Translation annotations is not yet solved in potx, but I feel it would be orders of magnitude easier to parse our strings wrapped in @Translation vs. trying to identify what kind of annotation this is and then what keys should be saved for translation from that kind of annotation (with static code analysis).

I would also point out it's a little bit of a DX-WTF if I only have to wrap certain strings in @Translation but others I shouldn't because it's taken care of for me. So me writing a new plugin, how do I know which should I use? I would rather we suffer and just have @Translation defined in each annotation rather than trying to make it automatic, and the job of potx harder.

StatusFileSize
new55.82 KB
PASSED: [[SimpleTest]]: [MySQL] 54,443 pass(es).
[ View ]

Okay, so well abandon that avenue for now. Still an improvement!

Issue tags:-Plugin system

+++ b/core/lib/Drupal/Core/Entity/Annotation/EntityType.phpundefined
@@ -0,0 +1,255 @@
+class EntityType extends Plugin {

Just wondering whether all controllers should be defined in the same place in this class.

Assigned:Unassigned» tim.plunkett
Status:Needs review» Postponed
Issue tags:+Plugin system

This contains the patch in #1967420: Allow Core\AnnotatedClassDiscovery to pass all parameters to the constructor of Component\AnnotatedClassDiscovery

I copied all of the properties directly in the EntityManager, in the same order. Yes the order doesn't make sense yet. Yes some of the properties don't make sense :)

Status:Postponed» Needs work
Issue tags:+Annotation

Okay, work to do.

Status:Needs work» Needs review
StatusFileSize
new2.48 KB
new55.13 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/lib/Drupal/Component/Annotation/Plugin.php.
[ View ]

Okay! This lets us kill the defaults processing too.

Status:Needs review» Needs work

The last submitted patch, entity-type-1967294-18.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new55.17 KB
FAILED: [[SimpleTest]]: [MySQL] 53,976 pass(es), 474 fail(s), and 58 exception(s).
[ View ]

Whoops. Forgot you can't do that:

diff --git a/core/lib/Drupal/Component/Annotation/Plugin.php b/core/lib/Drupal/Component/Annotation/Plugin.php
index 32535ad..82dd1af 100644
--- a/core/lib/Drupal/Component/Annotation/Plugin.php
+++ b/core/lib/Drupal/Component/Annotation/Plugin.php
@@ -36,7 +36,8 @@ class Plugin implements AnnotationInterface {
    * classed annotations that were used.
    */
   public function __construct($values) {
-    $defaults = new \ReflectionClass($this)->getDefaultProperties();
+    $reflection = new \ReflectionClass($this);
+    $defaults = $reflection->getDefaultProperties();
     // Remove the protected $definiton from the defaults.
     unset($defaults['definition']);
     $parsed_values = $this->parse($values);
ph

This is so many flavors of awesome that we should make a sundae bar.

Status:Needs review» Needs work

The last submitted patch, entity-type-1967294-20.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.39 KB
new54.72 KB
FAILED: [[SimpleTest]]: [MySQL] 54,469 pass(es), 10 fail(s), and 0 exception(s).
[ View ]

Okay, showing off more of my inexperience with Reflection. This one should be good to go now.
Also, accidentally added a default for render_controller_class that didn't belong.

+++ b/core/lib/Drupal/Component/Annotation/Plugin.phpundefined
@@ -34,7 +36,13 @@ class Plugin implements AnnotationInterface {
-    $this->definition = $this->parse($values);
+    $reflection = new \ReflectionClass($this);
+    // Only keep actual default values by ignoring NULL values.
+    $defaults = array_filter($reflection->getDefaultProperties(), function ($value) {
+      return $value !== NULL;
+    });
+    $parsed_values = $this->parse($values);
+    $this->definition = NestedArray::mergeDeep($defaults, $parsed_values);

As discussed on IRC, we could also use get_object_vars($this) here, but well, that's just an option.

+++ b/core/lib/Drupal/Core/Entity/Annotation/EntityType.phpundefined
@@ -0,0 +1,262 @@
+ * Contains Drupal\Core\Entity\Annotation\EntityType.

"\"

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined
@@ -171,7 +47,10 @@ class EntityManager extends PluginManagerBase {
-    $this->discovery = new AnnotatedClassDiscovery('Core', 'Entity', $namespaces);
+    $annotation_namespaces = array(
+      'Drupal\Core\Entity\Annotation' => DRUPAL_ROOT . '/core/lib',
+    );
+    $this->discovery = new AnnotatedClassDiscovery('Core', 'Entity', $namespaces, $annotation_namespaces, 'Drupal\Core\Entity\Annotation\EntityType');

It's great that we don't need another discovery class, as this would be confusing for some people.

+++ b/core/lib/Drupal/Core/Entity/Annotation/EntityType.phpundefined
@@ -0,0 +1,262 @@
+ * Contains Drupal\Core\Entity\Annotation\EntityType.

"\"

Status:Needs review» Needs work

The last submitted patch, entitytype-1967294-23.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new54.72 KB
PASSED: [[SimpleTest]]: [MySQL] 54,451 pass(es).
[ View ]
new675 bytes

Ugh, I copypasted the default for $field_cache wrong, switching it back to TRUE as it is currently in HEAD.

Also, added the missing \ that @dawehner pointed out, thanks!

Talked to beejeebus about the Reflection vs get_object_vars(), he said not to change it unless we have documented performance reasons, since this is cleaner.

Status:Needs review» Reviewed & tested by the community

This looks like a HUGE improvement to my eyes. RTBC!

Eclipse

Just in general I'm wondering whether/how we should document that a specific value is optional (for example render_controller_class).

Status:Reviewed & tested by the community» Needs work

When we document parameters on hook_foo_info() and the like, we just prefix their description with "(optional) ".. we should probably do that here as well, so we don't lose that information.

Status:Needs work» Needs review
StatusFileSize
new4.69 KB
new56.53 KB
PASSED: [[SimpleTest]]: [MySQL] 54,359 pass(es).
[ View ]

Okay, added in the (optional) information that was in EntityManager, and converted the brand spankin new Field and FieldInstance entities! Yay!

If it comes back green, I say RTBC.

Eclipse

♥♥♥

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Fixed

F*ck yeah! This looks great!!

Committed and pushed to 8.x! :D Yay for a big step in the right direction of mere mortals being able to grok D8. :)

Minor : unless I'm mistaken, If EntityManager has no more $defaults, it doesn't need ProcessDecorator() anymore ?

You're unfortunately mistaken, we still have all of this noise:

public function processDefinition(&$definition, $plugin_id) {
    parent::processDefinition($definition, $plugin_id);
    // Prepare entity schema fields SQL info for
    // Drupal\Core\Entity\DatabaseStorageControllerInterface::buildQuery().
    if (isset($definition['base_table'])) {
      $definition['schema_fields_sql']['base_table'] = drupal_schema_fields_sql($definition['base_table']);
      if (isset($definition['data_table'])) {
        $definition['schema_fields_sql']['data_table'] = drupal_schema_fields_sql($definition['data_table']);
      }
      if (isset($definition['revision_table'])) {
        $definition['schema_fields_sql']['revision_table'] = drupal_schema_fields_sql($definition['revision_table']);
      }
    }
  }

Now, half of our entity types don't need that, maybe we could move that into DatabaseStorageController where it belongs.

Priority:Normal» Critical
Status:Fixed» Active
Issue tags:+Needs change record

Doesn't this need change notice updates?

I've updated https://drupal.org/node/1827470, I think that's enough?

Assigned:tim.plunkett» Unassigned
Priority:Critical» Normal
Status:Active» Fixed
Issue tags:-Needs change record

Okay, and this one, we should be good now
https://drupal.org/node/1400186

Automatically closed -- issue fixed for 2 weeks with no activity.