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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
Issue tags: +Plugin system
FileSize
63.6 KB

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

msonnabaum’s picture

Yes! This looks great.

tim.plunkett’s picture

So, instead of

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

we will have

/**
 * @EntityType(
 *   id = "node",
 *   label = "Node"
 * )
 */
Dave Reid’s picture

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

tim.plunkett’s picture

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.

tim.plunkett’s picture

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.

webchick’s picture

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.

dawehner’s picture

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

webchick’s picture

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

webchick’s picture

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

tim.plunkett’s picture

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?

Gábor Hojtsy’s picture

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

Dave Reid’s picture

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.

tim.plunkett’s picture

FileSize
55.82 KB

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

dawehner’s picture

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.

tim.plunkett’s picture

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

tim.plunkett’s picture

Status: Postponed » Needs work
Issue tags: +Annotation

Okay, work to do.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.48 KB
55.13 KB

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
55.17 KB

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
xjm’s picture

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.39 KB
54.72 KB

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.

dawehner’s picture

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
54.72 KB
675 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.

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

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

Eclipse

dawehner’s picture

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

webchick’s picture

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
4.69 KB
56.53 KB

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

EclipseGc’s picture

If it comes back green, I say RTBC.

Eclipse

msonnabaum’s picture

♥♥♥

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

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

yched’s picture

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

tim.plunkett’s picture

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.

xjm’s picture

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

Doesn't this need change notice updates?

tim.plunkett’s picture

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

tim.plunkett’s picture

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

tim.plunkett’s picture

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