API page: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Form%21Co...

ConfigFormBaseTrait::config()'s @return doxygen states that it returns an ImmutableConfig object, but it can also return an editable config object (Config).

CommentFileSizeAuthor
#4 2684319-4.patch1.23 KBtraviscarden
#2 2684319-2.patch888 bytestraviscarden

Comments

TravisCarden created an issue. See original summary.

traviscarden’s picture

Assigned: traviscarden » Unassigned
Status: Active » Needs review
StatusFileSize
new888 bytes
jhodgdon’s picture

Status: Needs review » Needs work

Thanks!

+++ b/core/lib/Drupal/Core/Form/ConfigFormBaseTrait.php
@@ -41,8 +41,10 @@
-   * @return \Drupal\Core\Config\Config|\Drupal\Core\Config\ImmutableConfig
-   *   A configuration object.
+   * @return \Drupal\Core\Config\Config|\Drupal\Core\Config\Config|\Drupal\Core\Config\ImmutableConfig
+   *   An editable configuration object if the given name is listed in the
+   *   getEditableConfigNames() method or an immutable configuration object if
+   *   not.

Um...

So it looks like the @return line already has both Config and ImmutableConfig in it. The new @return line in the patch has added Config in there twice?

I like the added docs, because they clarify when each one will be returned, but the changes to @return line itself seem to be wrong.

Also... Do you think we could fix up this paragraph that is further up in the docs header?

   * Objects that use the trait need to implement the
   * \Drupal\Core\Form\ConfigFormBaseTrait::getEditableConfigNames() to declare
   * which configuration objects this method returns override free and mutable.
   * This ensures that overrides do not pollute saved configuration.
 

I cannot understand what it means. Actually, it seems like the docs you have added to @return kind of restate this. So maybe just take this out?

traviscarden’s picture

Status: Needs work » Needs review
StatusFileSize
new1.23 KB

I agree. That other paragraph is confusing and redundant. Updated patch attached.

jhodgdon’s picture

Version: 8.0.x-dev » 8.2.x-dev
Status: Needs review » Reviewed & tested by the community
Issue tags: +needs backport to 8.0.x, +needs backport to 8.1.x

Looks good, thanks!

This should be OK for 8.0, 8.1, and 8.2. I'll set this to 8.2 with backport tags, and set off a test against 8.2.

  • alexpott committed 8e2fa49 on 8.2.x
    Issue #2684319 by TravisCarden, jhodgdon: ConfigFormBaseTrait::config...

  • alexpott committed 341e132 on 8.1.x
    Issue #2684319 by TravisCarden, jhodgdon: ConfigFormBaseTrait::config...

  • alexpott committed ea35e64 on 8.0.x
    Issue #2684319 by TravisCarden, jhodgdon: ConfigFormBaseTrait::config...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed ea35e64 and pushed to 8.0.x, 8.1.x, and 8.2.x. Thanks!

Status: Fixed » Needs work

The last submitted patch, 4: 2684319-4.patch, failed testing.

alexpott’s picture

Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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