Posted by xjm

Problem

When reading the Plugin definition for Node.php (or other entities), developers are likely to look up the chain of inheritance for documentation. In this case, Node extends Entity. However once on the Entity class, there are no docs because the documentation is on the EntityManager (which provides all the defaults of Entity plugins). Hence, we should add a @see reference to where all the docs are.

Original Report

Just for you @quicksketch.

CommentFileSizeAuthor
#10 1881794-10.patch1.55 KBxjm
entity-docs.patch474 bytesxjm

Comments

quicksketch’s picture

My IDE doesn't yet know how to read PSR-0 style @see references. But this should help in any case find the documentation for the plugin definition.

Some additional background, this came up from a discussion about where PHPdoc for plugin definitions should live. Previously *_info() style hooks documented their structure in hook_*_info() example functions located in [module].api.php files. In most places in D8, this documentation has gone missing. I personally think documentation like this should go on the Base class directly (that is on Entity in this case), since an IDE (or developer) is more easily able traverse up and down the inheritance tree than follow every possible @see reference for documentation.

In this case, I *think* api.drupal.org is also able to link such @see references. So maybe for now we should call this good rather than moving all the docs.

Status: Needs review » Needs work

The last submitted patch, entity-docs.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review

Bot goof.

api.d.o currently searches for the class at the end of a PSR-0 namespace in order to link it, but @jhodgdon has said that it will be updated in the future.

With regard to why the docs for the class settings keys are on the manager and not the base class, it's because the manager provides the defaults that are listed in that documentation. See: EntityManager::$defaults

quicksketch’s picture

I see that the defaults are in EntityManager (and that you can link directly to those defaults, which is a plus), but all the defaults are already specified in the documentation. And the location of the defaults is again specified in the documentation already:

 * The defaults for the plugin definition are provided in
 * \Drupal\Core\Entity\EntityManager::defaults.

Just because the defaults are specified in the same file doesn't mean it's the most logical place to put the documentation. As a developer may be regularly extending entities, the Entity class is going to be in my regular toolkit, while the EntityManager will only be modified and referenced by developers changing the Entity system itself. From a downstream developer's perspective, the documentation is difficult to find, while an upstream developer working on core and the entity system might find this documentation to be perfectly located.

This might be an incorrect comparison, but you might compare this documenting private functions (staring with an underscore), rather than documenting public API functions. The private function (or Manager class in this case) will only be modified by people working on actual subsystem itself, while the public API may be used by a larger audience. Even though the private function may do the majority of the ugly work, if its never invoked directly by developers, it doesn't occur to developer to look at it for documentation. The documentation should be on the public code that they're meant to reference and may look to for examples.

xjm’s picture

Well, to me, the manager is the most important class because that's where all the decorators get added, but I agree I'm probably looking at the base class more frequently once I know what the manager does. Feel free to file a patch to move it if you feel it should be moved. :) Entity was the first time we had to do this, so there was nothing else to model it on. I really don't feel strongly about it; I'm just explaining why the decision was made originally.

See also: #1871762: Add detailed documentation explaining the block system's architecture

effulgentsia’s picture

I think it's good practice for all plugin types to also come with an interface they expect their plugins to conform to. For example, in core we have EntityInterface, FetcherInterface, etc. Seems like that might be a logical place to document the definition keys. However, not every definition key is relevant to the interface. For example, almost all plugins, regardless of interface, have 'id', 'title', and 'description' as definition keys. These are usually not for the benefit of the plugin class, but for code that works with the manager (i.e., a configuration form for selecting which plugin to use). From that perspective, documenting the definition keys on the manager makes sense.

Maybe documenting the definition keys on the manager class, but having the interface contain an @see annotation to the manager would be best.

effulgentsia’s picture

Title: Link the EntityManager from the Entity base class » Link the EntityManager from the Entity base class (or EntityInterface?)
xjm’s picture

I considered the interface when I was working on the EntityManager docs, but the interface doesn't/shouldn't know anything about the plugin system. So I don't think defining the keys there is ideal, though we should certainly link the object where they're defined. The fact that Entity and EntityInterface didn't was an oversight.

In #1871762: Add detailed documentation explaining the block system's architecture I've taken a stab at adding some documentation on the base plugin, and I think that might make sense, since developers will always be extending the base class to create their plugins. That's also what the plan is for Views, eventually. Entity is a little different because it itself isn't abstract.

xjm’s picture

Filed #1883744: Convert Entity to EntityBase for consistency with ContentEntityBase and ConfigEntityBase to try to capture why documenting the definition on Entity seems weird currently.

xjm’s picture

StatusFileSize
new1.55 KB

Meanwhile, more links all around.

jhodgdon’s picture

I don't see any strong reason not to commit this patch -- adding a few @see lines should not cause any problems.

But given the discussion above, I'm wondering if something more than an @see would be helpful? @see doesn't tell you *why* you might want to look at these other classes, it just makes a link. It could be more helpful to say something like "See the documentation page for \Drupal\Core\Foo for an explanation of bar." or "... for a list of the bar properties" or whatever is appropriate.

So I'll leave this open for consideration of that rather than just committing it right now...

jhodgdon’s picture

Status: Needs review » Needs work

Guess I need to change the status in order for my previous comment to be noticed. :)

xjm’s picture

Assigned: Unassigned » xjm
xjm’s picture

sun’s picture

Component: documentation » plugin system
jhodgdon’s picture

Why did you move this to "plugin system"? It's a pure documentation issue as far as I can see?

sun’s picture

We currently have a lot of documentation-related issues with the new plugin system. People who are concerned about the DX and documentation aspects of plugins are looking into the plugin system component, not the documentation component.

xjm’s picture

Assigned: xjm » Unassigned
xjm’s picture

Issue summary: View changes

More verbosity.

xjm’s picture

xjm’s picture

Issue summary: View changes

Removing myself from the author field so that I can unfollow the issue. --xjm

tim.plunkett’s picture

Component: plugin system » entity system
Issue summary: View changes

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

berdir’s picture

Status: Needs work » Closed (outdated)

I'm just going to close this one, EntityManager has been split up in many classes and I don't think we want to add @see for all of them.