Follow up for #1869250-37: Various EntityNG and TypedData API improvements
Problem/Motivation
Various EntityNG and TypedData API improvements has several segments of code that are confusing, especially concerning why they do what they do.
Proposed resolution
Add inline comments to clarify what they do and why.
Remaining tasks
To be specified yet. (reviews needed, tests to be written or run, documentation to be written, etc.)
User interface changes
None.
API changes
None.
Original report by @sun
From: #1869250-37: Various EntityNG and TypedData API improvements
The only more general issue I had while reviewing is that a lot of the new (and existing) entity data handling methods are performing very complex operations, but they do not contain any inline code comments that would explain what is being done and more importantly why. Therefore, I stared on a couple of code blocks and tried to mentally decipher what the code is doing, and why, and whether the code is correctly doing what it attempts to do, but in almost all cases, I ended up with "Hrm... whatever, I hope it's ok."
The only other and partially more elaborate comments are actually @todo comments... and most of those are even harder to decipher for someone who's not neck-deep into the code ;)
Could we create a separate, dedicated issue for improving the technical inline code documentation? And/or alternatively, can we make sure to vastly improve the docs through other issues/patches? :)
Comment | File | Size | Author |
---|---|---|---|
#59 | d8_docu.interdiff.txt | 753 bytes | fago |
#59 | d8_docu.patch | 140.02 KB | fago |
#56 | d8_docu.interdiff.txt | 57.95 KB | fago |
#56 | d8_docu.patch | 140.02 KB | fago |
#52 | d8_docu.patch | 98.16 KB | fago |
Comments
Comment #1
fagoStarted 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.
Comment #2
fagoComment #4
tim.plunkett#1: d8_docu.patch queued for re-testing.
Comment #5
BerdirNot sure if that's already covered but this is an example of an actual comment that is problematic:
> // 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.
Comment #6
sunPosted another review in #1869250-49: Various EntityNG and TypedData API improvements
Comment #7
fagoResponding to sun's comment over at #1869250-47: Various EntityNG and TypedData API improvements:
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" ?
Watch out - this determines the bundle - i.e. the value - for the to be created entity, whereas the bundleKey well is the bundleKey.
Generic code should not hard-code the "value" field component, it might not be there.
Added a comment.
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.
Improved that comments.
Expanded the comment.
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?
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.
fixed.
fixed
We don't support that right now.
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.
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)
Comment #8
fagoComment #9
fagotodo: Go through the Entity system and improve comments.
Comment #10
fagoNoted I missed some comments from sun, addressing the remaining comments:
fixed
It isn't? We are cutting of the first char, expanded the comment there.
improved comment
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.
Comment #11
fagook, 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?
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?
Comment #13
fago#11: d8_docu.patch queued for re-testing.
Comment #14
plachI 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.
Comment #15
fagoYep, please review ASAP. We shouldn't let this lie around long such that we avoid running in conflicts...
Comment #16
plachI think this hould be: "as providing references to these arrays would make $entity->values and $entity->fields reference themselves"
"Any field value"
Wrong wrapping
Why is this needed?
(here and below)
Better? "untranslatable" enity fields...
Is this intended?
Comment #17
fagoYes, figured out it's duplicating the parent method.
"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.
Comment #18
tim.plunkettAh, 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?
Comment #19
fagoThanks - 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.
Comment #20
plachThere is a minor thing missing from the previous review ("these arrays"):
The patch does not apply anymore btw.
Comment #21
fagoops, missed that sry. Re-rolled and updated patch.
Comment #22
plachI 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.
Comment #23
plachComment #24
webchickSweet merciful crap on a cracker. :)
Tossing to jhodgdon since I can no longer keep track of what all of our various documentation standards are.
Comment #25
jhodgdonThis 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
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:
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
Should be:
An entity field; that is, a list of field item objects.
Comment #26
BerdirThat'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.
Comment #27
jhodgdonOh. Can you put that into the documentation? I didn't have a clue about that from reading what is in this patch. :)
Comment #28
YesCT CreditAttribution: YesCT commentedI ran into this the other day too. (or I thought I did in #1807692-39: Introduce a column synchronization capability and use it to translate alt and titles through the image field widget. That was about what NG means.)
I think BC means backward compatibility.
I support spelling that out (*joke*) either by spelling it out or adding explanation.
Decorator sounded like "improved" so I looked it up
http://en.wikipedia.org/wiki/Decorator_pattern
seems a standard term that does not need to be documented. But probably contributed to the impression that we would *want* to use this BC Decorator.
Comment #29
fagoThanks 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.
Yep, let's discuss. For now I've left it out as suggested.
Comment #31
fagocomment conversion just went in - re-rolled the patch.
Comment #33
fago#31: d8_docu.patch queued for re-testing.
Comment #34
YesCT CreditAttribution: YesCT commentedLooks like the documentation stuff has been addressed. Back to rbtc.
Comment #35
jhodgdonThanks 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.
Comment #36
webchickHm. I thought the code changes were purely coding standards-related things (self:: vs. static:: etc.) but maybe I'm wrong.
Comment #37
jhodgdonThere are code changes like this:
No idea what is happening here...
Comment #38
fagoYes, 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.
Comment #39
plachI confirm the code changes are good.
Comment #40
jhodgdonI 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?
Comment #41
sunThanks 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:
Whereas, if we want to make D8 successful, then we need to get that to:
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. ;)
Comment #42
jhodgdonI 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?
Comment #43
fagoI'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.
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.
Comment #44
fagoComment #45
BerdirImprovements 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 :)
Comment #46
jhodgdonIMO, 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:
*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.
Comment #47
fagoYes, 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?
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 :(
Comment #48
jhodgdonRE #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?
Comment #49
jhodgdonHow 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?
Comment #50
BerdirNG 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.
Comment #51
jhodgdonOK... Well, here are two concrete things I would like to see in this patch:
a) Right now there are:
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.
Comment #52
fagook, updated the patch.
Comment #53
jhodgdonThanks! 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.
Comment #54
EclipseGc CreditAttribution: EclipseGc commentedWhy 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
Comment #55
EclipseGc CreditAttribution: EclipseGc commentedOn 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
Comment #56
fagoThis 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:".
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.
Good catch, fixed.
Attaching updated patch in the hope it's ready now.
Comment #57
fagoThis 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.
Comment #58
klausiShould 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.
Comment #59
fagoGood catch. I've just incorporated the fix, see attached patch/interdiff.
Comment #60
sunI agree we should be moving forward here. If any further tweaks are needed, we can do them in a separate follow-up issue.
Comment #61
effulgentsia CreditAttribution: effulgentsia commentedAccording to #1845546-84: Implement validation for the TypedData API, this is blocking that issue, so raising this to major.
Comment #62
webchickOk, committed and pushed to 8.x to keep things rolling. Thanks all!
Comment #64
alexpottCleaning up tags