Problem/Motivation

The configuration events system is a bit unwieldy are does not provide important information. For example when the SAVE event is fired it is hard to find what data has actually changed. The event names are magic strings and there is no explicit event fired when config is renamed. Even worse, when config is renamed, overrides are not applied and when new configuration is created, overrides are not properly merged.

Proposed resolution

This patch:

  • adds an event interface so we can add further events easily
  • provides a method to events to easily identify if a value changed
  • provides a class of constants for event listeners so developers can easily use the system
  • adds a rename event as currently no events are fired when this occurs
  • fixes to apply overrides when renamed happen
  • fixes to apply overrides when creating new configuration
  • adds comprehensive tests for the events

Remaining tasks

Review patch. Commit.

User interface changes

None.

API changes

1. Instead of the magic strings, use constants from the ConfigEvents class to identify events.
2. A new ConfigEvents::RENAME event is added and fired when a config object is renamed.
3. Config event implementations should implement/expect ConfigEventInterface.
4. A new changed() method is added to Config events so that certain value changes can be inspected/identified.

Files: 
CommentFileSizeAuthor
#50 2175917.50.patch23.5 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 64,332 pass(es). View
#50 47-50-interdiff.txt1.15 KBalexpott
#42 interdiff.txt831 bytesjibran
#38 interdiff.txt929 bytesGábor Hojtsy
#38 config_system_cleanup-2175917-38.patch23.8 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 63,759 pass(es). View
#27 interdiff.txt2.91 KBGábor Hojtsy
#27 config_system_cleanup-2175917-27.patch23.79 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 63,418 pass(es), 3 fail(s), and 0 exception(s). View
#22 config_system_cleanup-2175917-23.patch24.32 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 63,677 pass(es). View
#22 interdiff.txt3.47 KBGábor Hojtsy
#21 interdiff.txt1.68 KBGábor Hojtsy
#21 config_system_cleanup-2175917-21.patch23.8 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 63,697 pass(es), 3 fail(s), and 1 exception(s). View
#19 interdiff.txt6.68 KBGábor Hojtsy
#19 config_system_cleanup-2175917-19.patch23.62 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 63,676 pass(es). View
#14 config_system_cleanup-2175917-14.patch21.53 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 63,673 pass(es), 1 fail(s), and 0 exception(s). View
#13 config_system_cleanup-2175917-13.patch21.61 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 63,687 pass(es), 1 fail(s), and 0 exception(s). View
#10 config_system_cleanup-2175917-9.patch19.06 KBpiyuesh23
PASSED: [[SimpleTest]]: [MySQL] 63,147 pass(es). View
#8 config_system_cleanup-2175917-7.patch18.99 KBpiyuesh23
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#5 config_system_cleanup-2175917-5.patch19.86 KBpiyuesh23
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config_system_cleanup-2175917-5.patch. Unable to apply patch. See the log in the details link for more information. View
#1 2175917.1.patch17.51 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 59,941 pass(es). View

Comments

alexpott’s picture

Status: Active » Needs review
FileSize
17.51 KB
PASSED: [[SimpleTest]]: [MySQL] 59,941 pass(es). View

Patch attached begins to do what is outlined in the issue summary.

alexpott’s picture

Title: Clean Configuration system events » Clean up Configuration system events
Sutharsan’s picture

  1. +++ b/core/lib/Drupal/Core/Config/Config.php
    @@ -533,7 +544,7 @@ public function getStorage() {
       protected function notify($config_event_name) {
    -    $this->eventDispatcher->dispatch('config.' . $config_event_name, new ConfigEvent($this));
    +    $this->eventDispatcher->dispatch($config_event_name, new ConfigEvent($this));
       }
     
    

    Do we still need the notify method? In other places in core we call eventDispatcher->dispatch() directly.

  2. +++ b/core/lib/Drupal/Core/Config/ConfigEvents.php
    @@ -0,0 +1,34 @@
    + * Created by PhpStorm.
    + * User: alex
    + * Date: 17/01/2014
    + * Time: 15:24
    + */
    +/**
    

    You deserve credits, but not in this way ;)

  3. +++ b/core/lib/Drupal/Core/Config/ConfigEvents.php
    @@ -0,0 +1,34 @@
    +  const RENAME = 'config.rename';
    ...
    +}
    \ No newline at end of file
    

    Missing new line.

  4. +++ b/core/lib/Drupal/Core/Config/ConfigEvents.php
    @@ -0,0 +1,34 @@
    +  /**
    +   * @see \Drupal\Core\Config\Config::save()
    +   */
    +  const SAVE = 'config.save';
    

    Pls add more comment to these constants. This class is the most logical place to add event documentation, so lets use it. In the documentation we should also mention that different event objects are use for different events
    For a documentation example see: Symfony\Component\HttpKernel

  5. +++ b/core/lib/Drupal/Core/Config/ConfigEvents.php
    @@ -0,0 +1,34 @@
    +class ConfigEvents {
    

    There are two more comment related events:
    - config.importer.validate
    - config.importer.import
    I think we should add them here too.

  6. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -307,10 +307,11 @@ public function rename($old_name, $new_name) {
    +    // Prime the cache and laod the configuration with the correct overrides.
    +    $config = $this->get($new_name);
    
    +++ b/core/modules/config/tests/config_events_test/lib/Drupal/config_events_test/EventSubscriber.php
    @@ -0,0 +1,61 @@
    +} ¶
    \ No newline at end of file
    
  7. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -307,10 +307,11 @@ public function rename($old_name, $new_name) {
    +    // Prime the cache and laod the configuration with the correct overrides.
    

    Typo: 'laod'

piyuesh23’s picture

Assigned: Unassigned » piyuesh23
piyuesh23’s picture

FileSize
19.86 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config_system_cleanup-2175917-5.patch. Unable to apply patch. See the log in the details link for more information. View

Fixed the review comments mentioned in the patches above. Uploading a new patch for review.

Status: Needs review » Needs work

The last submitted patch, 5: config_system_cleanup-2175917-5.patch, failed testing.

piyuesh23’s picture

Status: Needs work » Needs review
piyuesh23’s picture

FileSize
18.99 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

My bad..:( uploaded the wrong patch here. Updating the patch again.

Status: Needs review » Needs work

The last submitted patch, 8: config_system_cleanup-2175917-7.patch, failed testing.

piyuesh23’s picture

Status: Needs work » Needs review
FileSize
19.06 KB
PASSED: [[SimpleTest]]: [MySQL] 63,147 pass(es). View

Missing second argument to Event Dispatcher. Uploading the another patch.

xjm’s picture

This is either critical, or not a beta blocker. Which? :)

Sutharsan’s picture

Issue tags: +Needs reroll

Patch no longer applies.

Gábor Hojtsy’s picture

Title: Clean up Configuration system events » Clean up configuration system events
Issue summary: View changes
Issue tags: -Needs reroll +Spark
FileSize
21.61 KB
FAILED: [[SimpleTest]]: [MySQL] 63,687 pass(es), 1 fail(s), and 0 exception(s). View

Rerolled the patch. The originalData concept has been introduced on Config already in #2108599: Convert language_default to CMI so no need to introduce here. The rest of the patch still makes total sense. I also applied ConfigEvents constants to the import events where it was not yet applied. Now there should not be any events *not* using the constants.

I was not 100% sure about the changes around the rename events, since that was also recently changed in HEAD to do the load(), so I did not modify how that works just added the event. Will see how that works :)

Also updated the issue summary for the current scope now that originalData is already in core but the rest of the clenaups and added tests are not. Working on this as part of my Spark work, so adding that tag.

Gábor Hojtsy’s picture

FileSize
21.53 KB
FAILED: [[SimpleTest]]: [MySQL] 63,673 pass(es), 1 fail(s), and 0 exception(s). View

Wups, just adding the three missing newlines at end of files.

Note that I'm also not sure about the docs approach used to document the the config events **on the constants for their name**, but not my idea :D Looks odd that the constants would have params and return values. Do we have an approach for documenting these things?

alexpott’s picture

Priority: Major » Critical
Status: Needs review » Needs work

This is critical because we're fixing an problem with applying overrides to renamed config objects and making small but necessary API changes to make the configuration system consistent.

This patch introduces ConfigEvents - a class which contains constants for all the configuration system events. We already have a class calls ConfigEvent - this patch should rename that to ConfigCrudEvent as this will make the distinction between ConfigEvents, ConfigImporterEvent and ConfigCrudEvent clear.

We need to test that overrides are applied to a renamed configuration object. They currently are not.

Adding documentation about the event to ConfigEvents is a great idea but actually adding @param and @return to class constants is wrong. We should add an @see to code where the event is dispatched from and an @see to an example implementation (if one exists).

The last submitted patch, 14: config_system_cleanup-2175917-14.patch, failed testing.

The last submitted patch, 14: config_system_cleanup-2175917-14.patch, failed testing.

The last submitted patch, 13: config_system_cleanup-2175917-13.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
23.62 KB
PASSED: [[SimpleTest]]: [MySQL] 63,676 pass(es). View
6.68 KB

The fail was easy to fix. The test was asserting the original value for a new config object would be array(), while it is in fact NULL. This is a question of definition, I think it makes sense to distinguish the "new config" case from the "empty config" case with this (and this is already the existing core behaviour, so not really my decision anyway).

Renamed ConfigEvent to ConfigCrudEvent, hopefully not missed anything.

alexpott’s picture

Status: Needs review » Needs work

So we still need to add test about overrides and renames. Overrides are config name specific so we need to test that the new overrides are applied and old overrides are not after a rename. With the current code in HEAD no overrides will be applied.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
23.8 KB
FAILED: [[SimpleTest]]: [MySQL] 63,697 pass(es), 3 fail(s), and 1 exception(s). View
1.68 KB

All right, here is a patch that WILL FAIL then. It showcases two bugs:

1. When the config object did not exist before, you cannot apply overrides. This is shown by the exception. Because NULL (original value) is attempted to being merged with the override array when looking at original data. Not a good idea :D So we either need to cast to array there or move the original data to default to an empty array. I plan to chose the later later.

2. Overrides are indeed not applied to the renamed config as the 3 fails show.

Gábor Hojtsy’s picture

FileSize
3.47 KB
24.32 KB
PASSED: [[SimpleTest]]: [MySQL] 63,677 pass(es). View

So the solution to the merge exception is to merge arrays with arrays. Defaulting the original data to an empty array works for new config objects. Fixed the assertion to assert for the empty array then.

The solution to the rename override issue was supposed to be that I reuse get() on the factory, which would apply the override, but then I figured out the new key needs a new override (which will apply to both original/new data, since we rename, there is no data change) and the raw data will obviously not have the override. So fixing those too. This passes and proves the rename applies overrides as well :)

Now the event docs need to be cleaned up (not today :) and then this should be good to go, no?

The last submitted patch, 21: config_system_cleanup-2175917-21.patch, failed testing.

The last submitted patch, 21: config_system_cleanup-2175917-21.patch, failed testing.

Sutharsan’s picture

Symfony event classes and the Drupal\Core\Routing\RoutingEvents class use final class for the event defining class to prevent it from being extended. Should we do this here too?

Gábor Hojtsy’s picture

@Sutharsan: our rename event actually extends from our base crud event :) So we could not mark any final except the rename event. Not sure if this makes sense in this context.

Also our CrudEvent is somewhat misleadingly named then :) It is invoked on creation and update (both cases as save) and delete, but NOT on read. So its more like a CUD not a CRUD :D Anyway, I don't have a better suggestion :)

Gábor Hojtsy’s picture

Issue summary: View changes
FileSize
23.79 KB
FAILED: [[SimpleTest]]: [MySQL] 63,418 pass(es), 3 fail(s), and 0 exception(s). View
2.91 KB

All right, updated the docs as suggested. I only @see-d actual runtime implementations, not test implementations, which I think makes sense to learn actual use.

@alexpott: looks good now? :) I'd be happy to put the icing on and write a change notice draft to get this committed :D

Gábor Hojtsy’s picture

Added draft change notice at https://drupal.org/node/2189269.

Status: Needs review » Needs work

The last submitted patch, 27: config_system_cleanup-2175917-27.patch, failed testing.

Gábor Hojtsy’s picture

Fails on imagefielddisplaytest array_flip(): Can only flip STRING and INTEGER values! array_flip(Array) Drupal\Core\Entity\FieldableDatabaseStorageController->loadMultiple(Array) Don't think that could be related.

Gábor Hojtsy’s picture

The last submitted patch, 27: config_system_cleanup-2175917-27.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Status: Needs work » Needs review

Finally passes green :) Did not pick up a stray fail this time. Quick, RTBC? :)

Gábor Hojtsy’s picture

Status: Needs review » Needs work

The last submitted patch, 27: config_system_cleanup-2175917-27.patch, failed testing.

alexpott’s picture

The test fail will be due to $conf being removed

Gábor Hojtsy’s picture

Assigned: piyuesh23 » Unassigned
Status: Needs work » Needs review
FileSize
23.8 KB
PASSED: [[SimpleTest]]: [MySQL] 63,759 pass(es). View
929 bytes

All right, updating for $config. I'm using $GLOBALS directly because obviously $config is a variable we often use in local scopes for configuration system objects (like for example here as well). So the global variable named the same is not very ideal IMHO, but that is what we have.

Gábor Hojtsy’s picture

jibran’s picture

Minor coding issues.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEventsTest.php
@@ -0,0 +1,91 @@
+

+++ b/core/lib/Drupal/Core/Config/ConfigEvents.php
@@ -0,0 +1,60 @@
+

Extra space.

Gábor Hojtsy’s picture

@jibran: can you help fix that and upload a fixed version soon? I may not have time for that soon :/

jibran’s picture

FileSize
831 bytes
23 KB
PASSED: [[SimpleTest]]: [MySQL] 63,766 pass(es). View

I have fixed the doc issue but I don't know that it will help the issue in moving forward. I left the useless review because I thought that current approach in the patch still need more discussion. :)

Gábor Hojtsy’s picture

Erm, what needs more discussion?

jibran’s picture

nvm :)

alexpott’s picture

FileSize
1.99 KB
23.16 KB
PASSED: [[SimpleTest]]: [MySQL] 64,188 pass(es). View

The more I think about the more Im convinced the having an interface for an Event is incorrect. Let's have less code to maintain.

Sutharsan’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Config/ConfigCrudEvent.php
    @@ -0,0 +1,57 @@
    +  public function changed($key) {
    +    return $this->config->get($key) !== $this->config->getOriginal($key);
    +  }
    

    I would rather change this name to isChanged(). Although changed() is not ambiguous, we have many more is* methods in core and $event->isChanged('langcode') (in ConfigSubscriber) is IMHO slightly better explaining itself.

  2. +++ b/core/lib/Drupal/Core/Config/ConfigRenameEvent.php
    @@ -0,0 +1,45 @@
    +  public function __construct(Config $confg, $old_name) {
    +    $this->config = $confg;
    +    $this->oldName = $old_name;
    +  }
    

    Typo in $fconfg (2x).

  3. One event listener left unchanged: Drupal\language\EventSubscriber\ConfigSubscriber::getSubscribedEvents():
      static function getSubscribedEvents() {
        $events['config.save'][] = array('onConfigSave', 0);
        return $events;
      }
alexpott’s picture

Status: Needs work » Needs review
FileSize
2.17 KB
23.48 KB
PASSED: [[SimpleTest]]: [MySQL] 64,329 pass(es). View

Thanks @Sutharsan!

Patch addresses your points.

beejeebus’s picture

Status: Needs review » Reviewed & tested by the community

spent some time looking through this patch. looks great :-)

catch’s picture

Status: Reviewed & tested by the community » Needs review
  1. +++ b/core/lib/Drupal/Core/Config/ConfigEvents.php
    @@ -0,0 +1,59 @@
    +  const RENAME = 'config.rename';
    

    Extremely minor but why not next to save/delete?

  2. +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
    @@ -258,12 +258,10 @@ public function rename($old_name, $new_name) {
    +    $config = $this->get($new_name);
    

    Is this actually priming the cache? The old code was writing to $this->cache, but no sign of that here.

  3. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEventsTest.php
    @@ -0,0 +1,90 @@
    +  public static $modules = array('config_events_test');
    

    We're using a DUTB/a module here to get events registered. Not sure if any of our existing tests do this, but feels like it ought to doable to register a listener in PHPUnit instead. Would save the test module and allow for an actual unit test then.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.15 KB
23.5 KB
PASSED: [[SimpleTest]]: [MySQL] 64,332 pass(es). View

1. Sure
2. Yes - this is calling ConfigFactory::get() which goes through exactly the same code path as any retrieval from the configuration system.
3. I want the initiative to tackle unit testing once beta is in - there'll be quite a bit of work here to get coverage and mocking working.

Setting back to RTBC because all I've done is move code around.

Gábor Hojtsy’s picture

@catch: on 3, I would add that some of the events only have implementation in this module. I think it is better to have them use the full apparatus that any other module would need to, it is easier to discover for developers looking for sample implementations. Somewhat contradictory, I said in #27:

All right, updated the docs as suggested. I only @see-d actual runtime implementations, not test implementations, which I think makes sense to learn actual use.

If the value of the test implementation shows better by linking it from the docs / if you think this would be helpful, then we can do that too IMHO. The ones that don't have @see for implementation in ConfigEvents.php don't have implementation outside of tests.

catch’s picture

Title: Clean up configuration system events » Change record: Clean up configuration system events
Priority: Critical » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Missing change record

Committed/pushed to 8.x, thanks!

alexpott’s picture

Title: Change record: Clean up configuration system events » Clean up configuration system events
Status: Active » Fixed
Issue tags: -Missing change record
catch’s picture

Priority: Major » Critical
Sutharsan’s picture

I've created two follow-up issues:

Gábor Hojtsy’s picture

There was also a change record draft at https://drupal.org/node/2189269, the contents of which I integrated into Alex's: https://drupal.org/node/2193665/revisions/view/6905619/6933559 and then deleted.

Gábor Hojtsy’s picture

Closed down #1889474: Create tests for config event handling as a duplicate. Thanks @tim.plunkett for the tip :)

Status: Fixed » Closed (fixed)

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