Improve comments and clean-up code for EntityNG and the TypedData API

Started over and generally improved comments + added some inline comments for the typed data API. Inline comment improvements for EntityNG are todo - I will come back to it later.

#1: d8_docu.patch queued for re-testing.

Not sure if that's already covered but this is an example of an actual comment that is problematic:

     foreach ($records as $id => $record) {
-      $entity = new $this->entityClass(array(), $this->entityType);
-      $entity->setCompatibilityMode(TRUE);
-
+      $values = array();
       foreach ($record as $name => $value) {
-        $entity->{$name}[LANGUAGE_DEFAULT][0]['value'] = $value;
+        // Avoid unnecessary array hierarchies to save memory.
+        $values[$name][LANGUAGE_DEFAULT] = $value;
       }
-      $records[$id] = $entity;
+      $bundle = $this->bundleKey ? $record->{$this->bundleKey} : FALSE;
+      // Turn the record into an entity class.
+      $records[$id] = new $this->entityClass($values, $this->entityType, $bundle);
     }

> // Avoid unnecessary array hierarchies to save memory.

This comment only makes sense in the diff, when it is an explanation why this differs to the old code. When you just look at the new code, it is just confusing IMHO :) It should explain *what* it is leaving away to avoid unecessary hierarchies.

Posted another review in #1869250-49: Various EntityNG and TypedData API improvements

Responding to sun's comment over at #1869250-47: Various EntityNG and TypedData API improvements:

1) What kind of additional optional definitions are added here, and why do they overlap with the bundle definitions? Why are additional values from an optional definition for a bundle key left out and not merged in (due to +=)?

It adds in per-bundle fields - or optional fields. The term optional might be a bit miss-leading here, as it's not necessarily optional for an entity. The field is optional for the entity type as it only applies dependent on the bundle. Let's find a better name for this - maybe "bundle fields" ?

The comment can be removed. Or perhaps replaced in order to explain why $this->bundleKey is re-evaluated here - isn't exactly this negotiation happening in __construct() of a storage controller already? I.e., isn't $this->bundleKey == $bundle anyway already?

Watch out - this determines the bundle - i.e. the value - for the to be created entity, whereas the bundleKey well is the bundleKey.

Why are the passed in values to create() set directly as property $name, whereas all other code is using $name->value?

Generic code should not hard-code the "value" field component, it might not be there.

It's not clear why the value assignment does not account for multiple value records.

Added a comment.

It's not clear to me what this comment tries to communicate and educate about. The (logical/linguistic) context and topic is missing. It also sounds/feels like the comment rather belongs onto a method's phpDoc block instead of the class level (though not sure).

Yep, that rather complex comment explains a rather complex issue why we cannot another simpler-looking way to access the field/values property. Improved and moved comment.

Who or what is setting $this->decorated->fields[$name] (the corresponding __set() method does not), and why can this code unset that property without re-instantiating it?

Improved that comments.

I don't really understand why this is copying values from one language to another, whereas there are actually no values for the other language?

Expanded the comment.

$definition seems to be unused? Do we need a hasProperty() method?

Maybe, but I do not see any harm in having the unused definition (it doesn't need to be assigned). But yeah, we could have that kind of syncatic sugar - not sure it's needed though?

Is there any reason for why we allow $this->list to be NULL at any time?

Yes, we had it implemented as you suggest previously but then it turned out we actually have a valid use-case for differentiating, see #1868274: Improved EntityNG isset() test coverage.

Some code is accessing $this->list directly, while some other code is using the $this->list() method - the majority of code accesses the property directly instead of using the method, so I think we want to change the instances that call into the method.

fixed.

Most foreach loops call the list values $item, while __clone() calls it $property.

fixed

Wouldn't a star ('*') be more appropriate? 0 will prevent an individual list item to have a diverging data type, no?

We don't support that right now.

E.g., if someone were to write a entity_reference_ng module in contrib that allows to reference multiple different entity types, then we'd have a list of varying data types, or am I mistaken?

Once we'd register each entity type as data type, potentially. But not in the current design as the entity reference would be a list of field-items with each of the same type.

This loop and value assignment seems to diverge from the regular entity storage controllers? (i.e., this is the same controller method I mentioned at the beginning, but this one does not omit [0]['value'])

True - not sure how this evolved. Anyway, that code gets generalized by #1833334: EntityNG: integrate a dynamic property data table handling. Let's make sure this gets fixed there. (The latest patch already does)

todo: Go through the Entity system and improve comments.

Noted I missed some comments from sun, addressing the remaining comments:

1) Typo in "$langocde" ($langcode).

fixed

2) Why is the langcode trimmed to 2 characters? That's a pretty bold assumption...?

It isn't? We are cutting of the first char, expanded the comment there.

"usually needed" by what and why?

improved comment

According to this revised implementation, getValue() will return NULL if an item is empty, so why do we check for isEmpty() first?

No, empty() and NULL is not the same as in PHP. There may be an item which has e.g. array('value' => NULL) as value, which is treated as empty() but is not NULL. The isEmpty() method corresponds to the field_is_empty() hook/callback of field api.

ok, worked through the entity system to improve comments and add more inline-comments. Interdiff and updated patch attached.

Do we have a doxygen convention for the following?

* @var TypedDataInterface[]

This is the way I've seen it in symfony. Not sure we do this yet, but I think we should such that we can denote the expected classes inside an array?

#11: d8_docu.patch queued for re-testing.

I totally wish to review this ASAP to become more confortable with the new stuff. Sorry for missing the parent issue: I didn't realize it was about introducing the BC decorator.

Yep, please review ASAP. We shouldn't let this lie around long such that we avoid running in conflicts...

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.php
@@ -63,17 +65,32 @@ public function getBCEntity() {
+    // do so, as providing references to this arrays would make $entity->values
+    // and $entity->fields to references itself, which is problematic during

I think this hould be: "as providing references to these arrays would make $entity->values and $entity->fields reference themselves"

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.php
@@ -63,17 +65,32 @@ public function getBCEntity() {
+      // Any field values set via the new Entity Field API will be stored inside

"Any field value"

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.php
@@ -94,6 +111,9 @@ public function __set($name, $value) {
+      // This is necessary as EntityNG does key values in default language always

Wrong wrapping

+++ b/core/lib/Drupal/Core/Entity/Field/Type/BooleanItem.php
@@ -28,12 +28,12 @@ class BooleanItem extends FieldItemBase {
-    if (!isset(self::$propertyDefinitions)) {
-      self::$propertyDefinitions['value'] = array(
+    if (!isset(static::$propertyDefinitions)) {
+      static::$propertyDefinitions['value'] = array(
         'type' => 'boolean',
         'label' => t('Boolean value'),
       );
     }
-    return self::$propertyDefinitions;
+    return static::$propertyDefinitions;

Why is this needed?
(here and below)

+++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityTranslation.php
@@ -15,17 +15,20 @@
+ * way as untranslated entity fields on the entity object.

Better? "untranslatable" enity fields...

+++ b/core/lib/Drupal/Core/TypedData/Type/Date.php
@@ -45,18 +45,4 @@ public function setValue($value) {
-  /**
-   * Implements TypedDataInterface::getString().
-   */
-  public function getString() {
-    return (string) $this->getValue();
-  }

Is this intended?

Is this intended?

Yes, figured out it's duplicating the parent method.

Why is this needed?
(here and below)

"self" is considered bad-practice as it has problems with late-static bindings, so this was criticised several times during reviews of unrelated code. Since php5.3 we have the keyword "static" which should be used instead.

I addressed everything else, patch attached.

Ah, I just opened #1882146: Subclasses of \Drupal\Core\Entity\Field\FieldItemBase should use static:: instead of self:: as a follow-up to #1839078: Implement the new entity field API for the email field type, maybe that should be marked a dupe and the email be rerolled to be in line with this issue?

Thanks - I marked #1882146: Subclasses of \Drupal\Core\Entity\Field\FieldItemBase should use static:: instead of self:: as duplicate of this.
Let's re-roll this or the e-mail patch depending on which issue goes in first.

There is a minor thing missing from the previous review ("these arrays"):

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.php
@@ -63,17 +65,31 @@ public function getBCEntity() {
+    // do so, as providing references to this arrays would make $entity->values

The patch does not apply anymore btw.

ops, missed that sry. Re-rolled and updated patch.

I think this is looking good, I'm going to RTBC it in a couple of days. I'l leave it NR for now so that @sun can have a look to it if he wishes to.

Sweet merciful crap on a cracker. :)

Tossing to jhodgdon since I can no longer keep track of what all of our various documentation standards are.

This patch has both code and documentation in it, so I'm assuming you just wanted me to review it and not commit it. :)

So, I did review the documentation. As far as standards go, the changes in the patch are OK. We still haven't agreed upon standards for using namespaces or not in documentation, but the one thing we've agreed upon is that if you do use a namespace in docs, it should start with a \ -- which this patch is doing well on.

I do have a couple of concerns about the content of the documentation (which could be addressed in a follow-up patch or preferably right now)... Setting back to "needs review" rather than "needs work" because I don't want anyone to get all angry at me for demanding better documentation...

a) In EntityBCDecorator.php

What exactly is a "BC Decorator"? I am not familar with the abbreviation "BC", so probably not everyone else reading this documentation would be either. Acronyms should be explained in documentation the first time they are used. Better yet, this class name should not be "EntityBCDecorator" at all, but instead have the acronym written out in words?

b) EntityBCDecorator again

As a minor detail, "e.g." means "for example", and it's preferable to just write out "for example" since everyone gets it confused with "i.e.". If you do want to use e.g., the correct punctuation would be:

Allows using entities converted to the new Entity Field API with the previous way of accessing fields or properties; e.g., via the BC decorator you can do:

Or better yet:
Allows using entities converted to the new Entity Field API with the previous way of accessing fields or properties. For example, via the BC decorator you can do:

c) EntityBCDecorator again

+ * @code
+ *   $node->title = $value;
+ *   $node->body[LANGUAGE_NONE][0]['value'] = $value;
+ * @endcode
+ * Without the BC decorator the same assignment would have to look like this:
+ * @code
+ *   $node->title->value = $value;
+ *   $node->body->value = $value;
+ * @endcode

Um. It is not clear to me how node->body[LANGUAGE_NONE][0]['value'] = $value; is related to $node->body->value = $value; -- where did all the language stuff go? Does this need an update of some kind? If not, some explanation of what happened to the language stuff might be useful? At a minimum, can I just say that these two code examples don't make me think that using the BC Decorator is a plus, because the second example sure looks easier for the Body field.

d) in EntityTranslation.php -- the docblock for the $fields property should start with a one-line 80-character-max summary. Probably you could put the information about what the array is in a separate line.

e) I noticed in FieldItemBase.php that someone did this:

-   * @var array<TypedDataInterface>
+   * @var TypedDataInterface[]

to indicate that the var was an array of TypedDataInterface items. But on EntityTranslation.php, the $fields property just says @var array. We don't have a standard supporting the syntax used in FieldItemBase:
http://drupal.org/node/1354#param-return-data-type
So probably this should be changed to @var array and the explanation of what it is should go on a separate line. We could also consider adopting a standard -- if so let's file a separate issue and also research how other projects document array data types like this, rather than inventing a new standard on this patch.

f) core/lib/Drupal/Core/Entity/Field/Type/Field.php

/**
- * An entity field, i.e. a list of field items.
+ * An entity field, i.e. a list of field item objects.

Should be:
An entity field; that is, a list of field item objects.

At a minimum, can I just say that these two code examples don't make me think that using the BC Decorator is a plus, because the second example sure looks easier for the Body field.

That's how it's supposed to be :) The BC decorator just exists so that you can access entities in the old way and only used while we convert stuff so that old code doesn't break and we don't have to convert everything at once.

Oh. Can you put that into the documentation? I didn't have a clue about that from reading what is in this patch. :)

Thanks for the new review. I've updated the patch to explain BC and to note it shouldn't be used if possible - see interdiff. I also updated the patch to include the regular static/self fixes for the recently committed email field item.

So probably this should be changed to @var array and the explanation of what it is should go on a separate line. We could also consider adopting a standard -- if so let's file a separate issue and also research how other projects document array data types like this, rather than inventing a new standard on this patch.

Yep, let's discuss. For now I've left it out as suggested.

comment conversion just went in - re-rolled the patch.

#31: d8_docu.patch queued for re-testing.

Thanks for addressing the documentation! Much improved.

I'm unassigning this from myself. This is code and docs changes so I won't be committing it.

Hm. I thought the code changes were purely coding standards-related things (self:: vs. static:: etc.) but maybe I'm wrong.

There are code changes like this:

-      if ($bundle && isset($this->entityFieldInfo['bundle map'][$constraints['bundle']])) {
-        $this->fieldDefinitions[$bundle] += array_intersect_key($this->entityFieldInfo['optional'], array_flip($this->entityFieldInfo['bundle map'][$constraints['bundle']]));
+      if ($bundle && isset($this->entityFieldInfo['bundle map'][$bundle])) {
+        $this->fieldDefinitions[$bundle] += array_intersect_key($this->entityFieldInfo['optional'], array_flip($this->entityFieldInfo['bundle map'][$bundle]));

No idea what is happening here...

Yes, there are some small code corrections also that were noted during reviews or I ran over while going through all the code. There are no functional changes though.

I confirm the code changes are good.

I cannot vouch for the accuracy of the in-code comments that are changing, but hopefully whoever is doing the code review can confirm this.

The documentation mostly follows our standards.... I gave the current patch a complete read-through though, and there are still some things that could still be cleaned up. They could be done in this round or a follow-up:

a) The first-line description for EntityBCDecorator should start with a verb.

b) There should be a blank line before the @todo at the end of EntityBCDecorator

c) The first-line description of EntityFormControllerNG should start with a verb and explain what NG is (like what was done to explain BC in EntityBCDecorator). I have no idea what NG is, and there is nothing with those initials in the description that I can see.

d) In EntityInterface, there is a mix of using namespaces and not using namespaces. We don't have a standard on whether to use them or not, but I found it quite jarring that sometimes they were used and sometimes they weren't. Probably it would be better to not mix them.

e) In EntityInterface, the @see should go at the end of the docs. If there is some contextual reason for wanting it where it is, then don't use @see (See Alsos are displayed at the end of docs on api.drupal.org, so if you want a reference to stay where it is, say something like "See the documentation for Foo for more information on bar" instead.)

f) Probably EntityNG should explain what NG is too?

g) In a code comment in EntityNG the word "fasten" is used ... I think it's meant to say that something becomes faster, but "fasten" doesn't mean that. :)

h) FieldItemInterface -- get() and set() methods -- first line verb tense (Gets/Sets vs. Get/Set)

i) Entity/Field/Type/Field -- first line description should start with a verb.

j) Same class -- the setField() method -- the param description says it can be NULL, so the type on the @param should be "array|null" not just "array".

k) Same class -- the descriptions of the getPropertyDefinition(s) and get* methods are ... not very descriptive. Delegates what to the first item of what?

Thanks all for these improvements. This patch looks ready to go for me.

That said, in general, I was rather looking for architectural/design docs that explain what the architecture is and why it was designed and implemented in this way. That's something you do not put into an online handbook or similar, but instead, is only of interest of developers who're staring at the code and who're trying to make sense of it.

The changes to the EntityInterface phpDoc block seem to go into the right direction, but we're obviously missing the larger, big picture, and how all of this is glued and tied together and supposed to work (as well as how it's not supposed to work).

In short, most of the EntityNG/TypedData architectural design is not really documented. As a developer, you're facing a massive amount of PHP classes that are stacked on top of each other, and you have no idea why that is.

Right now, if I wouldn't have been involved in most of this, then my initial developer learning experience will look like this:

  WTF?!? ------------------------------------------------------------------> WTF?

Whereas, if we want to make D8 successful, then we need to get that to:

  WTF?!? --> Odd. --> Ah-ha!

I don't know whether it is too early to think about these architectural design documentation aspects, but at some point in the very near future we'll have to get a bit more concrete on these things.

Additionally, we previously used @defgroup blocks in procedural include files for most of such documentation, but that no longer works with PSR-componentized code. It's possible that we additionally want to introduce README.md files for components, in order to document code/development aspects. But alas, that's an entirely separate issue of its own. ;)

I don't see why we can't use @defgroup documentation for these classes. None of them is a "Component" meant to be used outside Drupal -- they are all Drupal-specific "\Drupal\Core" classes.

So I think a @defgroup that gave us a roadmap of how to use the entity/field classes would be quite nice to have. It could go into entity.api.php (I think that exists somewhere?). But that might be a separate issue?

I'd love having per component README.md files, as it would make it much easier to read any inline documentation - but let's discuss that elsewhere.

So I think a @defgroup that gave us a roadmap of how to use the entity/field classes would be quite nice to have. It could go into entity.api.php (I think that exists somewhere?). But that might be a separate issue?

Agreed. Some of this is still subject of change, but anyway I'd help to document the status quo I think. Let's open a follow up for that.

ad #40: Thanks, I've updated the patch to address the additional points - interdiff attached. Also merged and fixed conflicts.

Improvements look good to me, back to RTBC.

@fago: @defgroups are parsed by api.module and show up as a topic on api.drupal.org, README's don't :)

IMO, this documentation about what NG means should be on all of the "NG" classes. Just putting @see (another class) is not very useful, because it doesn't tell you at all why you would want to look at the other class documentation.

I also just want to point out that this documentation:

  /**
-   * Delegates to the first item.
+   * Delegates to the first field item.
    */
   public function getPropertyDefinition($name) {

*still* doesn't tell me what the function does. The function presumably gets a property definition, but instead the documentation just says "Delegates to the first field item", which is something about *how* it does that, but doesn't document *what* the function actually does. Same comment still applies to all of those other "delegates" methods.

The function presumably gets a property definition, but instead the documentation just says "Delegates to the first field item", which is something about *how* it does that, but doesn't document *what* the function actually does. Same comment still applies to all of those other "delegates" methods.

Yes, but it tells you where to look for the actual documentation such as we do when implementing interface + makes clear this is actually just a short-cut. The alternative would be copying over loads of documentation to all those methods, but that would be a night-mare to maintain.
Or maybe we could just better describe it's a short-cut?

IMO, this documentation about what NG means should be on all of the "NG" classes. Just putting @see (another class) is not very useful, because it doesn't tell you at all why you would want to look at the other class documentation.

We can copy it around also, but anyhow I don't think we should invest lots of time in bullet-proofing documentation for classes that are only temporary here? I'd really prefer to move on to work on making those classes go away :(

RE #47 - Delegates -- No, it doesn't tell me where to look at all. "Delegates to the first field item" does not tell me at all which method on which class to look for to find the documentation. There needs to be a link to where the documentation is.

Regarding the "NG" - how long are they going to be around?

How about for the NG thing: instead of a @see EntityNG or whatever class it is now (which doesn't tell you why you'd want to look at that class), we put in a line that says "See the EntityNG documentation for an explanation of NG." or something like that?

NG as a term will go away relatively soon. Together with BC. It's just a temporary thing until we've converted everything. Then EntityNG will be merged into Entity and Entity will work like NG.

So the conceptual documentation of how it does/will work with NG-style will stay, but the term NG will be removed so it's not necessary to describe how it differentiates from non-NG.

OK... Well, here are two concrete things I would like to see in this patch:

a) Right now there are:

+ *
+ * @see \Drupal\Core\EntityNG

all over the place, because EntityNG has the explanation of what "NG" means. Could we change these lines to instead say:
See the EntityNG documentation for an explanation of "NG".
and put this line above where it is now (@see goes at the end; regular explanations go above). The @see does nothing for me, because I have no idea why I would want to look at the EntityNG documentation.

b) Fix the "Delegates to" method docs so that they actually say what the function does, such as "Gets the zyx by delegating to...", and so that they point to the class::method where the parameters and return value are documented. Right now they are doing neither (they are not saying what the function does, and they are not pointing to where the documentation is), so they are definitely violating our documentation standards.

ok, updated the patch.

Thanks! Much better.

There are still quite a few "Delegates" documented methods in FieldInterface.php that have not been fixed up by this patch.

Also in core/lib/Drupal/Core/Entity/Field/Type/Field.php, some of the comments use namespaces in the "Overrides/Implements" lines and some are omitting the namespaces. Let's try to be consistent. This may apply to other files in the patch too.

Also, there a few places in the patch where a @see self::something() was changed to @see static::something(), and in other cases, lines like this were changed to @see ClassName::something(). The latter style is much easier for the API module to deal with. Let's try to be consistent.

Why are the validate methods all slated to be removed in this code? That seems an api change worth discussing and one I really really intended on leveraging for the context API I'm working on over in #1896076: Contextual Plugins and supporting code.

Can we have a discussion about this before we just remove it? I was seriously considering filing a patch that filled all the validates in properly (or at least some of them).

Eclipse

On further reading it looks as though the validation is happening as part of the setValue() method. Is this a correct reading of the intent here? Having a separate validate method still seems sane to me, but I won't argue so long as there is some sort of validation going on here. A simple confirmation will set my mind at ease.

Eclipse

Can we have a discussion about this before we just remove it? I was seriously considering filing a patch that filled all the validates in properly (or at least some of them).

This patch does not remove validation, please check the interface. It's just removing a duplicated empty method. Validation is being developed over at #1845546: Implement validation for the TypedData API, so let's dicuss it there.

>There are still quite a few "Delegates" documented methods in FieldInterface.php that have not been fixed up by this patch.

Yeah, I left out the magic methods. Ok, so attached patch fixes them as well and unifies their docs to all start with "Magic method:".

Also in core/lib/Drupal/Core/Entity/Field/Type/Field.php, some of the comments use namespaces in the "Overrides/Implements" lines and some are omitting the namespaces. Let's try to be consistent. This may apply to other files in the patch too.

Indeed Implements wasn't totally consistent yet, thus I worked over it again and made all "Implements" references absolute. For Overrides I left the already applied rule of only making it absolute if the questioned class is not already imported/used as in that case it often refers to an override of the extending class - thus it should be consistent now.

Also, there a few places in the patch where a @see self::something() was changed to @see static::something(), and in other cases, lines like this were changed to @see ClassName::something(). The latter style is much easier for the API module to deal with. Let's try to be consistent.

Good catch, fixed.

Attaching updated patch in the hope it's ready now.

This will conflict with pretty much every other improvement to EntityNG and TypedData, thus marking accordingly. I'd suggest building any further patches upon this and move on with this fast to avoid more conflicts.

+++ b/core/lib/Drupal/Core/Entity/Field/Type/EntityTranslation.php
@@ -114,28 +118,28 @@ public function getProperties($include_computed = FALSE) {
   /**
-   * Magic getter: Sets the translated property.
+   * Magic getter: Sets a translated field.
    */
   public function __set($name, $value) {
     $this->get($name)->setValue($value);

Should be "Magic setter". Or actually "Magic method" to be consistent?

But otherwise looks really good. Considering the size of the patch and the urgency here I'm setting this to RTBC anyway. We can fix that one doc block in a follow-up.

Good catch. I've just incorporated the fix, see attached patch/interdiff.

I agree we should be moving forward here. If any further tweaks are needed, we can do them in a separate follow-up issue.

According to #1845546-84: Implement validation for the TypedData API, this is blocking that issue, so raising this to major.

Ok, committed and pushed to 8.x to keep things rolling. Thanks all!

Cleaning up tags