Problem/Motivation
Currently there is no standard way to document additional entity information provided by modules. As a consequence when the Content Translation module was added to core, all its specific entity information properties were documented in the ContentTranslationControllerInterface
(which does not feel the right place) and in part in the @EntityType
annotation, even if those are not supposed to be declared nor documented there, as contrib modules won't be able to do it. Moreover code in core/lib
should have no knowledge of code in core/modules
so this is plain wrong anyway.
Proposed resolution
- Remove the CT entity info keys from the
@EntityType
class. - Move the actual documentation in a place where people will actually find it, that is as a docblock in the
hook_entity_info_alter()
PHP doc. - Adopt this as an official documentation policy.
Remaining tasks
Documentation team reviewRTBC
User interface changes
None
API changes
None
Related Issues
#1967294: Add a dedicated @EntityType annotation
#1970360: Entities should define URI templates and standard links
Original report by tim.plunkett
As I discovered in #1967294: Add a dedicated @EntityType annotation, there are at least a half dozen properties in the annotation that are only used by translation_entity module.
The only properties in there should be for stuff relevant to \Drupal\Core.
Contrib modules will also likely want to tack on their own properties to the EntityType annotation, and translation_entity can serve as an example of how to do that.
Comment | File | Size | Author |
---|---|---|---|
#45 | ct-entity_info-1968970-45.patch | 9.78 KB | plach |
Comments
Comment #1
tim.plunkettOkay, let's do this.
Comment #2
Crell CreditAttribution: Crell commentedOr go all the way: #1970360: Entities should define URI templates and standard links :-)
Comment #3
plachWorking on this.
(Marked #1979152: Move translation_entity-specific entity type metadata out of the EntityType annotation as a duplicate)
Comment #4
plachFirst attempt.
Comment #5
tim.plunkettI don't think I was 100% clear with conveying my thought process, and even now I'm about to contradict what I said in another issue.
If I am writing a contrib entity, and I know I want to use translation_entity module, I *will* put this stuff in the annotation, why not?
None of these are harmful to have in there at all times, so they don't need a module_exists().
What I was hoping for was a clear way for modules to declare "Hey, I will look on the @EntityType for this key, it will do this"
Similar to how entity_metadata_hook_entity_info() was documented.
So a big docblock on translation_entity_entity_info(), some defaults added there, and leave the Comment.php and Node.php etc alone.
At the end, only the keys used by subsystems would be documented in EntityType.php, and things like menu_base_path and whatnot would be in their respective module's hook_entity_info() docblocks
Comment #6
plachWe already have a big docblock in EntityTranslationControllerInterface.php documenting ET keys. Do you think we should move it from there?
Comment #7
plachRemoved ET specific docs from the
EntityType
annotation and moved the related docs totranslation_entity_entity_info_alter()
.Comment #8
plachThis moves the docs into the
@file
section oftranslation_entity.module
.Not sure how to document this example. Perhaps just switch to hook_entity_info(), something like:
Comment #9
plachTrying to summon jhodgdon :)
Comment #10
Crell CreditAttribution: Crell commentedI think related, per conversations with Tim: #1970360: Entities should define URI templates and standard links
Comment #11
jhodgdonplach: as far as I know, no one uses the "documentation" tag in core issues. I at least do not follow it. Check the tag guidelines page link under the tags field. :) I do watch the "documentation" issue component.
Also, I've been sick a few days, so sorry for the delay once summoned by Druplicon in IRC... but I'm not sure what this issue is about really, so I'm not sure what I'm supposed to comment on? A summary would be helpful. :)
Comment #12
plach@jhodgdon:
Well deserved :)
Updated the issue summary. Long story short: we have a bunch of entity info keys that are understood only by the translation_entity module, we are not sure where we should document them.
Comment #13
jhodgdonWhat is this "developer" issue tag? I don't see that on the tag guidelines list either?
Anyway, thanks for the summary, but I'm still confused about both the summary and the documentation in the patch. When I read the patch:
- I don't get how putting this into the @file documentation there is going to help anyone find it. How about a @defgroup so it shows up as a Topic?
- This @file doc starts out "The entity translation UI relies on the entity info to provide its features." and then doesn't explain what "entity info" is or how to define it (a hook? plugin annotation?).
- Then the next paragraph starts out telling me that some keys are required but not all, and then gets really really confusing so that I can't even follow it. I think it would be better to just make a list of the keys (using our standard list formatting to indicate (optional) for optional keys and explaining for each key why/when it is really optional).
- The rest of the documentation in the @file just seems like a ramble... it really needs to be organized.
So... Can the documentation be rewritten so that it
a) Is in a @defgroup not @file, with relevant classes/hooks/whatever using @ingroup so they reference each other?
b) Actually tells a developer what they need to do to define a translatable entity class.
c) Is in a logical order.
Comment #14
plachI found it here: http://drupal.org/node/24565#tags, now I see those are tags for the documentation queue only. According to the topical issue tags page, the Documentation tag should be correct.
Please, let's leave alone the documentation content: if as it appears it needs heavy lifting, IMO we should do that in a dedicated issue (opened #1985232: content_translation_entity_info_alter() docs need reorg/rewrite). The main focus here is removing stuff from the
@EntityType
docs that does not belong there. The question is: where a (core/contrib) module is supposed to document entity info keys it provides for its own purposes?Comment #15
plachApparently it's impossible to submit a capitalized tag (Documentation) :(
Comment #16
jhodgdonRegarding the "documentation" issue tag, as it says on the Topic page:
"For Drupal core issues, select the Documentation component instead of using this tag. In general, component selection is preferred over tag selection." In other words, we don't use it in Core. Or at least, not officially, and I certainly don't check it. But whatever.
So... I don't get how the patch in this issue relates to the issue summary. I think the documentation that is in the patch should be in a @defgroup and not in a @file, and if you want to make a separate issue for cleaning up the docs, that is fine. And I still do not understand the question in #14, probably because I don't know enough about the plugin system or specifically translatable entity plugins. Sorry.
Comment #17
plachSorry for not being clear, I'll try to recap: an entity type is now defined through a specific
@EntityType
annotation. The annotation includes all the entity information that used to be provided byhook_entity_info()
earlier. However other modules can still augment this information throughhook_entity_info()
.The
translation_entity
module defines some info keys, specifically the'translation'
and the'menu_*'
ones, that contains some data it uses to generate a translation UI in a generic fashion.Since it's a core module, the integrating modules such as Node provide this information directly in their
@EntityType
annotations. Thetranslation_entity
keys ended up being documented in the@EntityType
class, where only core entity system stuff should be mentioned. We want to remove them for the reasons cited in the OP. But we don't know which should be the right place for thetranslation_entity
module to document these additional entity info keys.A contrib
foo
module wishing to augment core entity info keys, will implementhook_entity_info()
on behalf of the core entity-defining modules and will have to document these new keys it's defining. Both me and @tim.plunkett would look for such docs in afoo.api.php
file, but I was told this was not the right place when the initialtranslation_entity
patch was about to go in. So we are stuck.If you still think a
@defgroup
would be the best place, then I'm just creating it here and then we can revamp the docs in the other issue, without needing to block this one :)Comment #18
jhodgdonOK, thanks to timplunkett in IRC I think I have finally gotten my head around this, maybe anyway... So here's my suggestion:
a) Remove everything from the EntityType class docs that isn't required to define an entity. (Specifically, translation_entity module is not required, so any annotation keys specific to that need to be removed.)
b) Add a note in the EntityType class that additional annotation keys from core and contributed modules may be defined by hook_entity_info() implementations [?? is that right? or is it hook_entity_info_alter()] and maybe some information about how a module defining an entity type needs to put these in its @EntityType annotation, and how a contrib module could extend a core entity type to provide this kind of stuff for a core entity class (is that even possible)? That would tell people to look for hook_entity_info(_alter) implementations to discover additional EntityType annotation keys, I think?
c) And then I think each class that does this should define its annotation keys in its hook_entity_info(_alter) docs maybe? Does that make sense?
d) And (again if I've understood this correctly) we would also add a note in the base hook_entity_info(_alter) docs about this, saying that classes implementing that hook can use it to define additional @EntityType annotation keys?
Comment #19
tim.plunkettOptional keys should still be documented as such, but all of the keys should be ones from subsystems, nothing from modules.
b, c, and d seem right to me. Ideally the module would need to use hook_entity_info() anyway and the docs would be there, but if a module doesn't implement that hook and does implement hook_entity_info_alter(), that's a fine place too.
Comment #20
jhodgdonIt looks like translation_entity uses hook_entity_info_alter()?
Comment #21
plachYep, ET uses
hook_entity_info_alter()
because it's just providing defaults for its own keys. Actual values are supposed to be provided by modules integrating with it (through annotations orhook_entity_info()
).#18 + #19 sound like a plan to me :)
Comment #23
Gábor HojtsyComment #23.0
Gábor HojtsyAdded issue summary
Comment #24
plachThis should implement #18 + #19.
Comment #26
Gábor HojtsyLooks like this was still used? Undefined index: translation in content_translation_menu()
Comment #27
plachReverted the latest changes.
Comment #28
tim.plunkettNote, that last hunk is changed in #2133469: Replace path-based entity links with route names as well.
Comment #29
plachOk, I will fix this as soon as it goes in.
Comment #30
plachRerolled
Comment #31
plach30: ct-entity_info-1968970-30.patch queued for re-testing.
Comment #34
plach30: ct-entity_info-1968970-30.patch queued for re-testing.
Comment #35
Gábor HojtsyLooks good, except:
Let's add an issue link to the issue you reference in the comment. (If no issue yet, open one :).
Comment #36
plach... and fixed, thanks!
Comment #37
plachTagging as beta target since it would be better to provide developers the correct documentation.
Comment #38
plachAnd sprint obviously :)
Comment #39
Gábor HojtsyLooks all good now! Thanks!
Comment #40
xjm36: ct-entity_info-1968970-36.patch queued for re-testing.
Comment #41
plachComment #42
plachRerolled after #2005716: Promote EntityType to a domain object.
Comment #43
plachBetter merge
Comment #44
plachRetitled and updated the issue summary accordingly.
Comment #45
plachThese are now obsolete.
The interdiff is with #42.
Comment #47
plachGreen, back to RTBC.
Comment #48
webchickCommitted and pushed to 8.x. Thanks!
Comment #49
plachWoot :)
Comment #50
plachComment #51
Gábor HojtsyYay, thanks!