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 review
  • RTBC

User interface changes

None

API changes

None

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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Postponed » Active

Okay, let's do this.

Crell’s picture

plach’s picture

plach’s picture

Assigned: plach » Unassigned
Status: Active » Needs review
FileSize
14.21 KB

First attempt.

tim.plunkett’s picture

Title: Move translation_entity.module specific properties out of the Entity type annotation » Move translation_entity.module specific properties out of the @EntityType annotation
Status: Needs review » Needs work

I 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

plach’s picture

We already have a big docblock in EntityTranslationControllerInterface.php documenting ET keys. Do you think we should move it from there?

plach’s picture

Status: Needs work » Needs review
FileSize
10.23 KB

Removed ET specific docs from the EntityType annotation and moved the related docs to translation_entity_entity_info_alter().

plach’s picture

This moves the docs into the @file section of translation_entity.module.

+++ b/core/modules/translation_entity/translation_entity.module
@@ -3,6 +3,67 @@
+ * This is how entity info would look for a module defining a new translatable
+ * entity type:
+ * @code
+ *   function mymodule_entity_info_alter(array &$info) {

Not sure how to document this example. Perhaps just switch to hook_entity_info(), something like:

 * This is how entity info would look for a module defining translation support
 * for a "myentity" entity type:
 * @code
 *   function mymodule_entity_info(array &$entity_info) {
plach’s picture

Issue tags: +Documentation

Trying to summon jhodgdon :)

Crell’s picture

I think related, per conversations with Tim: #1970360: Entities should define URI templates and standard links

jhodgdon’s picture

Issue tags: -Documentation

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

plach’s picture

Title: Move translation_entity.module specific properties out of the @EntityType annotation » Remove translation_entity.module specific properties from the @EntityType annotation
Issue tags: +developer

@jhodgdon:

Check the tag guidelines page link under the tags field. :)

Well deserved :)

A summary would be helpful. :)

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.

jhodgdon’s picture

Status: Needs review » Needs work

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

plach’s picture

Issue tags: -developer +Documentation

What is this "developer" issue tag? I don't see that on the tag guidelines list either?

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

Anyway, thanks for the summary, but I'm still confused about both the summary and the documentation in the patch.

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?

plach’s picture

Apparently it's impossible to submit a capitalized tag (Documentation) :(

jhodgdon’s picture

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

plach’s picture

Sorry 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 by hook_entity_info() earlier. However other modules can still augment this information through hook_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. The translation_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 the translation_entity module to document these additional entity info keys.

A contrib foo module wishing to augment core entity info keys, will implement hook_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 a foo.api.php file, but I was told this was not the right place when the initial translation_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 :)

jhodgdon’s picture

OK, 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?

tim.plunkett’s picture

Remove everything from the EntityType class docs that isn't required to define an entity.

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

jhodgdon’s picture

It looks like translation_entity uses hook_entity_info_alter()?

plach’s picture

Yep, 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 or hook_entity_info()).

#18 + #19 sound like a plan to me :)

Gábor Hojtsy’s picture

Title: Remove translation_entity.module specific properties from the @EntityType annotation » Remove content_translation.module specific properties from the @EntityType annotation
Component: translation_entity.module » content_translation.module
Status: Needs review » Needs work
Gábor Hojtsy’s picture

Issue summary: View changes

Added issue summary

plach’s picture

Issue summary: View changes
Status: Needs work » Needs review
Related issues: +#1810350: Remove menu_* entity keys and replace them with entity links
FileSize
10.14 KB

This should implement #18 + #19.

Status: Needs review » Needs work

The last submitted patch, 24: ct-entity_info-1968970-24.patch, failed testing.

Gábor Hojtsy’s picture

Looks like this was still used? Undefined index: translation in content_translation_menu()

plach’s picture

Status: Needs work » Needs review
FileSize
1.63 KB
9.37 KB

Reverted the latest changes.

tim.plunkett’s picture

Note, that last hunk is changed in #2133469: Replace path-based entity links with route names as well.

plach’s picture

Ok, I will fix this as soon as it goes in.

plach’s picture

FileSize
9.37 KB

Rerolled

plach’s picture

Status: Needs review » Needs work

The last submitted patch, 30: ct-entity_info-1968970-30.patch, failed testing.

The last submitted patch, 30: ct-entity_info-1968970-30.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Looks good, except:

+++ b/core/modules/content_translation/content_translation.module
@@ -94,6 +118,8 @@ function content_translation_entity_info_alter(array &$entity_info) {
+        // @todo Remove this as soon as menu access checks rely on the
+        //   controller.
         $info['translation']['content_translation'] += array(

Let's add an issue link to the issue you reference in the comment. (If no issue yet, open one :).

plach’s picture

plach’s picture

Tagging as beta target since it would be better to provide developers the correct documentation.

plach’s picture

Issue tags: +sprint

And sprint obviously :)

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks all good now! Thanks!

xjm’s picture

plach’s picture

plach’s picture

FileSize
9.87 KB
plach’s picture

FileSize
1.4 KB
9.87 KB

Better merge

plach’s picture

Title: Remove content_translation.module specific properties from the @EntityType annotation » Standardize module-provided entity info documentation and clean-up the @EntityType annotation
Component: content_translation.module » entity system
Issue summary: View changes
Priority: Normal » Major

Retitled and updated the issue summary accordingly.

plach’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.57 KB
9.78 KB
+++ b/core/modules/system/entity.api.php
@@ -96,14 +96,15 @@ function hook_ENTITY_TYPE_create_access(\Drupal\Core\Session\AccountInterface $a
+ * as defined in the related annotation object.
...
+ * @see \Drupal\Core\Entity\Annotation\EntityType

@@ -114,6 +115,32 @@ function hook_entity_info(&$entity_info) {
+ * @see \Drupal\Core\Entity\Annotation\EntityType

These are now obsolete.

The interdiff is with #42.

The last submitted patch, 43: ct-entity_info-1968970-43.patch, failed testing.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Green, back to RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

plach’s picture

Woot :)

plach’s picture

Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, thanks!

Status: Fixed » Closed (fixed)

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