Problem/Motivation

Followup from #2232275: System menu blocks need to be able to depend on their menu entities.

Plugins can now declare configuration dependencies.

This should have been documented as part of committing the issue above, but the plugin annotation documentation seems to have disappeared. So part of this issue is finding the existing documentation and then, once found, updating it.

Proposed resolution

Find the existing plugin annotation documentation.
Updated it to include changes from #2232275.

Remaining tasks

User interface changes

API changes

Comments

xjm’s picture

Priority: Normal » Critical
Issue tags: +beta blocker
Related issues: +#2235363: Document config dependencies
xjm’s picture

jessebeach’s picture

Should I be editing these pages?

system.api.php -> https://api.drupal.org/api/drupal/core%21modules%21system%21system.api.p...

I don't see a @defgroup plugin yet.

jessebeach’s picture

jessebeach’s picture

Oy, I have no idea where to put this documentation. The use case and code is really particular. I thought that it might belong in API usage, but that's not right the more I think about it. Really, it feels like it should be a blog post. I guess I'll just continue with this page:

https://drupal.org/node/2235409

jessebeach’s picture

Status: Active » Needs review

Alright, this needs review. I tried to walk the line between illustrating how to declare the dependencies and where they are collected. I do not think this documentation should dive into block plugin derivatives and the inner workings of the specific implementation for system menu blocks.

https://drupal.org/node/2235409

xjm’s picture

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.

jessebeach’s picture

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.

I'd like to avoid turning this into an issue to document plugins in general. The intent is to document configuration dependencies. If I go down a rabbit hole of documenting plugins, I won't be able to focus on other blockers.

jessebeach’s picture

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

I've added a link to the configuration entity docs page.

I'm not sure I agree that we need to explain what not to do here. Declaring a dependency does not imply data mingling, so warning someone to avoid this just opens up a topic that I can't adequately address. Such detail is the stuff of blog posts and forum posts. We're just documenting capabilities here, right? Or I've completely misunderstood what you intended with the comment.

I'll expand the example of the menu configuration entity, since that's the only one we have right now.

jessebeach’s picture

This sentence doesn't provide context for what "another" configuration entity is

Actually, I'll just change the wording to an entity, full stop, and remove the suggestive grammar of "another".

jessebeach’s picture

Changed to: Plugins may also declare dependencies. A dependency might be a module, a theme or an entity.

jhodgdon’s picture

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!

xjm’s picture

Component: plugin system » documentation

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

jhodgdon’s picture

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

jessebeach’s picture

config_dependencies is a property that some configuration entities declare (in their schema currently). So whereas the property might be expressed in annotation, it isn't necessarily always defined in annotation. Annotation is (nearly) always a subset of the properties of a Plugin.

So config_dependencies is a logical property of a Plugin, which is why I was having trouble understanding where to put it. Maybe in the preamble to a Plugins section?

jhodgdon’s picture

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?

jessebeach’s picture

This doesn't sound like a generic Plugin thing then -- it's specifically a Config Entity thing, right?

Yes, you're right. Is this the config entity documentation? https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Config!Entity!Con...

jhodgdon’s picture

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?

jessebeach’s picture

FileSize
2.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,928 pass(es). View

Ok, how about this?

jhodgdon’s picture

Status: Needs review » Needs work

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?

jessebeach’s picture

Config and State topic instead

Hmm, I thought I had added it there! Sorry, I don't know how to view the changes I make to the API comments, so I don't know what they look like when rendered. I'm just kind of guessing :) Is there a way one can view these changes in a browser locally?

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.

So, I'm of the opinion that the current doc page is sufficient, that one at https://drupal.org/node/2235409

This topic is quite advanced. And one would not normally declare these dependencies in annotation, as @alexpott mentions here:

#2232275-24: System menu blocks need to be able to depend on their menu entities

@jessebeach well there would never be a config entity called menu.menu... this feature is much more likely to be used by derivatives that are based on a config entity like SystemMenuBlock where the instance definition and therefore config dependency can be worked out programmatically.

Which is a response to my tentative example:

/**
 * Defines a Block configuration entity class.
 *
 * @ConfigEntityType(
 *   id = "block",
 *   label = @Translation("Block"),
 *   config_dependencies = {
 *     "entity" = {
 *       "menu.menu"
 *     }
 *   }
 * )
 */

We have one example in the core codebase. As a developer, I would use this example to develop my own implementation. I don't think we can document this capability short of copy-pasting chunks of code into the doc page.

For me, I'd like to see a docs page that identifies the capability and points me to code that I can use as a starting point.

jhodgdon’s picture

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.

jessebeach’s picture

jessebeach’s picture

Status: Needs work » Needs review

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.

Ok, I see this point. I've tried to address the who, what, where and why of the issue in the original doc page: https://drupal.org/node/2235409

Upon further reflection, dependencies are a feature of configuration entities in general, not just plugins in particular, so this page should be needs to be reparented under https://drupal.org/developing/api/8/configuration. I don't have permission to do this.

I hid the patch on this issue. I'd prefer to leave the documentation for this capability on the standard community docs site.

jhodgdon’s picture

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.

xjm’s picture

Assigned: jessebeach » alexpott

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

dawehner’s picture

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

jessebeach’s picture

FileSize
1.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,979 pass(es). View

re:#27, dawehner, it turns out we have this documentation on \Drupal\Core\Config\Entity\ConfigDependencyManager. So I've added an @see in \Drupal\Core\Config\Entity\ConfigEntityInterface that links to it.

alexpott, I updated the docs page: https://drupal.org/node/2235409

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

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.

xjm’s picture

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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

xjm’s picture

Assigned: alexpott » Unassigned

Thanks!

xjm’s picture

Priority: Critical » Major

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.

xjm’s picture

Issue tags: -beta blocker +beta target

Actually changing the tag.

xjm’s picture

Title: Document how plugin definitions should declare config dependencies » Document config dependencies

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

alexpott’s picture

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

xjm’s picture

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.

Gábor Hojtsy’s picture

Assigned: Unassigned » Gábor Hojtsy
xjm’s picture

Priority: Major » Critical

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

jhodgdon’s picture

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.

xjm’s picture

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

jhodgdon’s picture

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

xjm’s picture

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

Berdir’s picture

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
5.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,315 pass(es). View
5.41 KB

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.

jhodgdon’s picture

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?

Gábor Hojtsy’s picture

Status: Needs review » Needs work

@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?

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
11.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,565 pass(es). View
11.73 KB

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.

Berdir’s picture

  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.

jhodgdon’s picture

FileSize
5.3 KB
11.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,373 pass(es). View

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.

jhodgdon’s picture

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

Gábor Hojtsy’s picture

Assigned: Gábor Hojtsy » Unassigned
Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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

EDIT: for clarity

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.51 KB
254.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,361 pass(es). View

Perhaps something like this.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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?

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
11.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,379 pass(es). View
3.47 KB

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]

Status: Needs review » Needs work

The last submitted patch, 57: 2235363-config-docs-56.patch, failed testing.

jhodgdon’s picture

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

alexpott’s picture

Status: Needs work » Needs review

Thanks @jhodgdon for fixing up my patch :)

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Great fixes :)

jhodgdon’s picture

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

webchick’s picture

Status: Reviewed & tested by the community » Fixed

On it! :)

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 1efaefb on 8.0.x
    Issue #2235363 by jhodgdon, jessebeach, alexpott, Gábor Hojtsy: Document...

Status: Fixed » Closed (fixed)

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