Document config dependencies

Oops.

Plugins may also declare dependencies. A dependency might be a module, a theme or another configuration entity.

This sentence doesn't provide context for what "another" configuration entity is. In general, we want the configuration entities to be decoupled from the plugin-configured data they store, and we probably need to explain that. It's a hard concept (see: months spent trying to decouple them for the Block API).

Looking promising other than that. I do think we also need to document this in the codebase, though. The problem is that the rest of the plugin API is also missing its codebase documentation. :P

I'd suggest looking for where the entity info annotation keys are documented these days; that should give us some idea of how to document plugin definition properties... or at least how it's been done so far.

Hi there! I've been out for the last 10 days... jessebeach left a message for me to ask about where to put these docs but I think maybe you found a good place? Let me know if you still have questions.

Regarding api.drupal.org, there are D8 topics called "Annnotations" and ... well we don't exactly have one for Plugins per se. There are topics about specific plugin types, like Blocks, but nothing called "Plugins" in the topics list or on the api.d.o landing page: https://api.drupal.org/api/drupal/8 -- could be added, or you could add to the Annotations page?

If you file a patch for this or any other api.d.o topic, please change the component to "Documentation". Thanks!

Agreed that this should be documentation component.

The problem is that this key needs to be documented in the codebase, like all keys in all plugin definitions. It's API, not just conceptual documentation. But unfortunately we're missing a good way of documenting these. The similar example I can think of is the entity info documentation, which I (re-)wrote for D8 back in 2012, and I can't even find it anymore. So we have a larger problem with documenting this metadata generally.

@alexpott also had some feedback on the documentation itself that's to be posted at some point.

Re: the absent plugins section, that's why I filed #2234439: Add a section for the plugin system to the API doc landing page. :)

OK -- hadn't seen the other issue yet (just got back from a long trip and I haven't looked through the new Docs component issues yet. Later today...)

So, we had an issue about documenting annotations several weeks ago, due to the EntityType problem.

We decided at that point that most annotations are (and should be) documented on their Annotation class, and all of these are listed at
https://api.drupal.org/api/drupal/core!modules!system!system.api.php/gro...
automatically by the API module (it makes all classes with @Annotation that are in /core but not /core/vendor show up in this topic). For most annotation classes, you can click through and get docs on the components of the annotation (the class members).

And for plugin/annotation types that uses get/set methods, such as EntityType, the plan was that when you click through to the annotation class, you will see something like this:
https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Entity!Annotation...
which tells you where to go to find documentation of this annotation type.

So... I'm a bit confused about this particular annotation thing of this issue, and how it fits into the picture... is config dependencies annotation common to all annotations? Is it on a class somewhere? ???

This doesn't sound like a generic Plugin thing then -- it's specifically a Config Entity thing, right? So it should probably go in documentation about config entities, not generic docs about Plugins?

Probably best to put information on the ConfigEntityInterface class, or perhaps in one of these topics (which are linked from the D8 api.d.o landing page):
https://api.drupal.org/api/drupal/core!modules!system!core.api.php/group...
https://api.drupal.org/api/drupal/core!modules!system!core.api.php/group...

Hmmm... Probably the Config & State topic will need to be expanded to make a sub-topic for config entities, and that would be a good place to put this information? Then both config_state and entity_api topics could link to this topic?

Hm. You've added this to the Entity API topic. Doesn't it belong in the Config and State topic instead? And... If I didn't know what this was to begin with (and I barely do)... I do not think this documentation would give me the information I need. I think it needs to start out with describing what a configuration dependency is, then how to declare it in the YML file, and then ... why even include that code block from Drupal\Core\Config\Entity\ConfigEntityBase::calculateDependencies() at all? It could refer to that if necessary, but what am I supposed to learn from reading that code? My eyes glossed over.

Also, nitpicks:
a)

+ * The config_dependencies key in the plugins' definition is assigned an array of
+ * dependencies. The keys of this array may be one of:
+ *   - entity
+ *   - module
+ *   - theme

Too much indentation. The - should line up with the text above.

b)

+ * For example, the system menu block for the Bartik theme declares the following
+ * dependencies in its configuration definition file.

Should end in : and not have a blank line afterwards?

c)

+ * // Drupal\Core\Config\Entity\ConfigEntityBase::calculateDependencies

Any use of a namespace in documentation *must* start with backslash.

d) See link to https://drupal.org/documentation/administer/config -- why is this relevant? This is presumably a programming topic, and the link is about how to administer configuration?

The only way to view what your changes would look like on api.drupal.org is to build an API site with the API module. Docs at: https://drupal.org/node/1516558 (not that I'd recommend it).

I still stand by my comment in #20. This documentation doesn't tell me why I need dependencies at all, what they're for, etc. Or how to do them, if they're normally not supposed to be in the YML file. And I don't see how the calculateDependencies() bit is useful at all. What is it supposed to illustrate?

What I'm trying to say is, after reading either the d.o page you linked or the patch (which appear to be identical), I still have no idea at all what this is about, whether I need to know about this, or if I did, how I would use it.

Thanks!

The page is reparented (block on the sidebar showing navigation is cached so it may take a moment to be fixed; this is a known and annoying bug that I wish would get fixed).

I agree with just putting this info on the d.o pages and not in a patch.

And the docs look a lot better, thanks! At least I have some idea after reading it now what is going on. I'd be inclined to mark this issue as fixed, but leaving for more knowledgeable others to review.

We need @alexpott to take a look at this -- he said he had some feedback on it as of last week.

+++ b/core/modules/system/core.api.php
@@ -120,6 +120,65 @@
+ *
+ * @section config_dependencies Configuration Dependencies
+ *
+ * Dependencies are declared in a plugin's definition. A system menu block is an
+ * example from Drupal core that utilizes this pattern. The menu block declares a
+ * dependency on the menu's corresponding entity. The following code illustrates
+ * how this association is established through the following method:
+ *
+ * @code
+ * // Drupal\Core\Config\Entity\ConfigEntityBase::calculateDependencies
+ * public function calculateDependencies() {

I would really rather have that directly on the interface. With @code we can place some examples in there as well. This would be the expected place to look for.

That @see looks quite reasonable in that patch... I think the online docs are also looking pretty good? Inviting anyone who thinks there are problems in the docs, or that we need more/better/additional docs, to set issue back to needs work and explain what still needs to be done.

@alexpott will review this for technical accuracy. He said he'd do so "this weekend".

The problem we have it that there still is nowhere in the code base that tells people about the special plugin definition config_dependencies key. Adding the @see is a good move. I guess we can add something to the text in ConfigDependencyManager.

I've also reshaped the text on the handbook page for correctness and added some @todo's

Thanks!

Discussed with @alexpott and we agreed to downgrade this from beta-blocking, since this particular documentation issue is not actually critical, and a non-release-blocker can't be a beta blocker by definition. The beta criteria also don't include missing documentation. However, we should still target it for beta if possible.

Actually changing the tag.

Per discussion with @alexpott and @tim.plunkett regarding #2248151: Configurable plugins can not control instance-specific config dependencies, expanding the scope of this issue.

So far the best documentation of configuration dependencies in the code base is https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Config%21...

From #1808248-137: Add a separate module install/uninstall step to the config import process:

No config uninstalls are not deferred. This is because they could affect config creation - that's why uninstalls are done first before installs. The other thing you is the final environment the config export came from - it is important to get to the same code base as soon as possible. I'm going to open an issue to add documentation to the ConfigImporter class about priority and ordering of everything.

Let's add that to the scope here too.

The more I talk to people about their CMI implementations, the more I think this issue is critical.

OK, ... What do we need to do here? I've read over all the comments on this issue and I'm a bit confused as to what we need to do to finish this issue.

We need to document the entire config dependency system. We rescoped the issue to that in #36.

I realize that from the issue title. What I don't know is what the remaining tasks are, what has been done, and where it needs to be documented. :)

https://www.drupal.org/node/2235409 is AFAIK all there is, and pretty much nothing in the codebase.

The documentation should also cover #2260457: Allow config entities to remove dependent configuration keys when dependencies are deleted due to module uninstall, which just got in.

Here is a substantial update. May or may not be entirely true :) Tried to build in the suggested stuff. I am not really clear on the soft-ness of the dependencies at different stages. As per @Berdir's issue it seems like the dependencies are quite hard (setting aside the new onDependencyRemoval()), but for installation, they are certainly very soft (ie. a view may depend on fields that are not even on the system as is currently for several core shipped views in the standard profile).

Interdiff vs. #28.

Please review.

Hm... A couple of concerns with this documentation:

a) I found this paragraph really difficult to understand:

+ * Dependencies in configuration do not supersede dependencies of containing
+ * modules/themes/profiles. For example if a view is shipped with an install
+ * profile and the view requires a 3rd party field module, the configuration
+ * will still be installed even if the profile does not require the module
+ * providing the field type. It is the responsibility of the containing project
+ * to depend on required modules. To include configuration that depends on
+ * optional 3rd party modules, add submodules that depend on that module in your
+ * project. The configuration dependency system is only used to manage ordering
+ * in the install/import processes and remove configuration in the module
+ * uninstall process.

Either the first two sentences say it all (in which case omit the rest), or ... well I couldn't understand the rest, no idea what other information it was trying to give me.

b)

+ * \Drupal\Core\Plugin\PluginDependencyTrait is also provided for convenience
+ * to calculate plugin based dependencies. Also used by configuration entities,
+ * this lets system menu blocks to depend on the displayed menu entity. This is
+ * crucial to ensure a configuration import will import the menu first before
+ * the block is saved on the system.
+ *

I don't understand why this is talking specifically about system menu blocks, without saying "for example" ... or are they a special case for some reason that I think isn't explained (at least not in this patch)? This patch is going into Drupal/Core/Config/Entity/ConfigDependencyManager.php, not a class that is specifically for system menu blocks...

c)

+ * may lead to unintended side effects (eg. node types and displays being
...
+ * dependencies is going to be removed (ie. if an entity is going to be deleted

Nitpick: Please don't use "eg." or "ie.". For one thing, the proper punctuation is "e.g.,", and for another, nearly no one understands the difference between e.g. and i.e. (you wouldn't believe how many issues with this I review/commit), so it's really just better to write it out as "for example" and "i.e." as "that is". Much clearer.

d)

+ *
+ * For example, entity displays remove references to widgets and formatters if
+ * the plugin that supplies them depends on a module that is being uninstalled.
+ *
 

This paragraph comes just after the explanation of the onDependencyRemoval() method, but ... it doesn't seem relevant, or else if it is, it needs to explain more. Are the entity displays implementing this method, or the widgets/formatters? Why not just let the standard removal process do this?

@jhodgdon: thanks for the review!

a) I would at least keep the first 3 sentences. While we can assume people reading this already understand module dependencies (and leave off the submodule example), I did not make that assumption and instead put it into perspective with an example. If you think you'd rather not have that example it is fine. I think the third sentence making it clearer that the containing module needs to have dependencies should be kept.

b) Should have used "For example" with the menu block. That is the only implementation in core right now, but may not be later on :)

c) Makes sense.

d) Erm, if you believe the standard removal process would work, then this example is not good enough yet. It is meant to explain that the standard removal process would remove all dependents entirely and this solution is to do removal of partial config as appropriate. Eg. if you remove a field, the whole entity display should not be removed (even though it depends on the field), but instead the field should be removed from the entity display. That is what this capability is about. What do you suggest, how should we get that through?

I got together with Gabor and Berdir in IRC and ... well, here's a new patch that hopefully clears things up a bit. Please check for accuracy and completeness!

As one note, ConfigEntityBase::addDependency() didn't have any docs -- the inheritdoc wasn't working because it was not technically an override of the trait method (the trait method was used with an alias). That is why I substituted that doc block.

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php
   @@ -14,37 +14,70 @@
   + * order. For example, node types are created before their field storages and
   + * the field storages are created before their fields.
   

   The example was there before, but it is wrong.

   field storages don't care about node types/bundles. There is no dependency between them.

   If you wanted a nested dependency examle then say that node types are created before their fields and both before their view display configuration.

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php
   @@ -14,37 +14,70 @@
   + * work without a certain field module, the profile must declare a dependency on
   + * this field module in its info file. If you find that you do not want your
   

   What exactly is meant with "field module" the module that provides the field type? (field is usually used for an actual field).

   Maybe field type module? Or split it up and first just talk about "depends on a certain field type", and later "declare a dependency on the module that provides that field type..." ?

3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php
   @@ -14,37 +14,70 @@
   + * dependencies coming from plugin bags and third party settings (and as one
   + * further note, plugins can declare configuration dependencies in their plugin
   + * definitions, with a 'config_dependencies' key). During dependency checking,
   

   This doesn't really make sense to me here (the config_dependencies part).

   I think plugin dependencies should be a separate paragraph, and here it would just reference to that like "from plugin bags (see below... ) and .."

   In there, I would say that plugins should define their dependencies either by putting it in the config_dependencies annotation key or by implementing ConfigurablePluginInterface::calculcateDependencies() so that config entities that use them can inherit the correct dependencies. If you want an example for all that, use blocks. The dependencies of blocks depend what kind of block (plugin) is used, a views block depends on the view (static annotatio key), another block plugin might allow you to chose what to display, resulting in a dynamic dependency.

4. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php
   @@ -14,37 +14,70 @@
   + * When an item that configuration can depend on is removed, such as when
   + * uninstalling a module, configuration that is dependent will also be
   

   Currently, uninstalling is the only thing that triggers this process, so "such as" is not quite correct. But we want to change there, there's an issue open somewhere to detect dependant config entities when config entities are deleted and allow to deal with them.

5. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php
   @@ -14,37 +14,70 @@
   + * as a node type configuration being entirely removed when the module defining
   + * a field it contains is uninstalled. To prevent this, configuration entity
   

   "node type configuration" is not really correct, we don't remove that. What we would remove is the view display configuration (which formatter to use for which field for a node type/view mode combination).

6. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php
   @@ -14,37 +14,70 @@
   + * list. So, in the above example, the node type configuration entity class
   + * should implement this method to remove field configurations when field
   + * modules are uninstalled, and entity display configuration classes should
   + * remove references to widgets and formatters if the plugin that supplies them
   

   same, entity view display... ah, you have that below already

   But still, node type configurations do not have dependencies on configurable fields.

   If you want a second example, that would be third party settings. if menu_ui adds per-node-type settings using third party settings, then uninstalling menu_ui would delete that node type.

   => Which is I think not actually implemented right now, should we? If so, then that would kill the use case in #2224581: Delete forum data on uninstall.

Thanks for the review in #50!

I think I've addressed all the points... Except #4, which I decided (since there could be more cases coming, and I don't think it's too confusing to have it there even if there aren't) to leave the "such as" in there.

Here's a new patch.

I just checked and this patch still applies cleanly. It's a critical but docs-only issue -- just needs a review...

Looked through the cross references and read the after-patch version. I think the improvements are great. Thanks!

I think we can refine to make the language a bit clearer.

EDIT: for clarity

Perhaps something like this.

Thanks. Found some grammar issues at least in my reading.

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php
   @@ -14,38 +14,35 @@
   + * If a configuration entity is provided as default configuration by an
   + * extension (modules, themes, or profiles), the extension has to depend on any
   

   an extension vs. "modules, etc" no matching plurality

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php
   @@ -14,38 +14,35 @@
   + * plugins and third party settings. To get a configuration entities current
   

   entity's no?

3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -367,6 +367,10 @@ public function link($text = NULL, $rel = 'edit-form', array $options = []) {
   +   * Note that this function should only be called from implementation of
   

   either "implementations of" or "an implementation of" no?

Umm... That patch has a LOT of stuff in it that is not related to this issue. Plus some minor copy editing issues (serial commas, possessives/plurals, etc.). Otherwise, very nice, thanks!

I rerolled it by going back to #51 and applying the interdiff. Then fixed the copy editing stuff. New patch attached.

[looks like I found/fixed the same things Gabor found, plus a couple more]

This triggered a (known, critical issue) random test failure. Retesting.

Thanks @jhodgdon for fixing up my patch :)

Great fixes :)

I guess both alexpott and I worked on this patch, so someone else should commit...

On it! :)

Committed and pushed to 8.0.x. Thanks!