Closed (outdated)
Project:
Drupal core
Version:
8.6.x-dev
Component:
entity system
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Anonymous (not verified)
Created:
7 Jan 2013 at 03:40 UTC
Updated:
26 May 2019 at 10:59 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
quicksketchMy 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.
Comment #3
xjmBot 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
Comment #4
quicksketchI 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:
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.
Comment #5
xjmWell, 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
Comment #6
effulgentsia commentedI 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.
Comment #7
effulgentsia commentedComment #8
xjmI 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.
Comment #9
xjmFiled #1883744: Convert Entity to EntityBase for consistency with ContentEntityBase and ConfigEntityBase to try to capture why documenting the definition on Entity seems weird currently.
Comment #10
xjmMeanwhile, more links all around.
Comment #11
jhodgdonI 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...
Comment #12
jhodgdonGuess I need to change the status in order for my previous comment to be noticed. :)
Comment #13
xjmComment #14
xjmFollowing #1831264: Use the Entity manager to create new controller instances and #1867228: Make EntityTypeManager provide an entity factory, moving the docs to Entity is probably does make sense.
Comment #15
sunComment #16
jhodgdonWhy did you move this to "plugin system"? It's a pure documentation issue as far as I can see?
Comment #17
sunWe 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.
Comment #18
xjmComment #18.0
xjmMore verbosity.
Comment #19
xjmComment #19.0
xjmRemoving myself from the author field so that I can unfollow the issue. --xjm
Comment #20
tim.plunkettComment #27
berdirI'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.