Clean up configuration system events

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
   

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

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

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

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

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

Patch no longer applies.

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.

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?

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.

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.

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?

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?

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

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

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

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.

27: config_system_cleanup-2175917-27.patch queued for re-testing.

27: config_system_cleanup-2175917-27.patch queued for re-testing.

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

27: config_system_cleanup-2175917-27.patch queued for re-testing.

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.

38: config_system_cleanup-2175917-38.patch queued for re-testing.

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.

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

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

Erm, what needs more discussion?

nvm :)

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;
     }

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.

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

Committed/pushed to 8.x, thanks!

I've created two follow-up issues:

• This issue added VALIDATE and IMPORT event name constants, but did not remove the associated event dispatchers in Drupal\Core\Config\ConfigImporter. #2195417: Clean up configuration import events
• I believe, my comment #25 got mis interpreted. This issue should make it clear: #2195407: Make ConfigEvents class a final class

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.

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