Problem/Motivation

#2175917: Clean up configuration system events Cleaned event constants for ConfigEvents::VALIDATE and ConfigEvents::IMPORT, but the event dispatchers were not converted.

Proposed resolution

Use ConfigEvents::* explicitly instead of building the event string in ConfigImporter::notify().

Remaining tasks

t.b.d.

User interface changes

none

API changes

The method ConfigImporter::getId() is removed.
The method ConfigImporter::notify() is removed - but it is protected so this is not an API change.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sutharsan’s picture

Status: Active » Needs review
FileSize
1.83 KB

ConfigImporter::notify() uses the ConfigImporter::ID to construct the event id string. I found that this ID was introduced in #1890784: Refactor configuration import and sync functions, but is now only used in ConfigImporter. This patch removes the usage of ID for events, and instead used ConfigEvents::IMPORT and ConfigEvents::VALIDATE.

alexpott’s picture

Issue summary: View changes
Status: Needs review » Needs work

ConfigImporter::ID was used to disambiguate installation and importing when they both used the ConfigImporter. This no longer occurs so lets clean that up here too.

Sutharsan’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
2.89 KB
1.86 KB

This patch also:

  • Removes the ConfigImporter::ID and replaces it with ConfigImporter::LOCKID for use with lock.
  • Removes the method ConfigImporter::getId() as it is no longer required.

The last submitted patch, 1: config-import-events-2195417-1.patch, failed testing.

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
@@ -227,7 +229,7 @@ public function validate() {
+      $this->eventDispatcher->dispatch(ConfigEvents::VALIDATE, new ConfigImporterEvent($this));

@beejeebus pointed out that VALIDATE could be confused with the CRUD events. Let's also rename this to IMPORT_VALIDATE

The last submitted patch, 3: config-import-events-2195417-3.patch, failed testing.

Gábor Hojtsy’s picture

Yeah naming the events even better +1 :) Thanks Sutharsan for working on this :)

Sutharsan’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
6.24 KB
4.37 KB

Changes in this patch:

  • ConfigEvents::VALIDATE renamed to ConfigEvents::IMPORT_VALIDATE
  • The method ConfigImporter::getId() is renamed to ConfigImporter::getLockId(). I removed getId in the previous patch, but the ID it was used to test the lock. Renamed the method to explains the meaning of the ID better.
alexpott’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -32,9 +33,9 @@
    -  const ID = 'config.importer';
    +  const LOCKID = 'config_importer';
    

    Convention is an underscore between words in class constants. So LOCK_ID

  2. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -294,33 +296,23 @@ protected function importInvokeOwner() {
    +  public function getLockId() {
    +    return static::LOCKID;
       }
    
    +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportUITest.php
    @@ -101,7 +101,7 @@ function testImportLock() {
    -    $config_importer_lock = $this->configImporter()->getId();
    +    $config_importer_lock = $this->configImporter()->getLockId();
         $this->container->get('lock')->acquire($config_importer_lock);
    

    Lets remove getLockId() too and just use ConfigImporter::LOCK_ID

Sutharsan’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
6.21 KB
2.74 KB

Comments in #9 implemented.

Status: Needs review » Needs work

The last submitted patch, 10: config-import-events-2195417-10.patch, failed testing.

Sutharsan’s picture

Status: Needs work » Needs review
FileSize
6.62 KB
935 bytes

Fixed a silly oversight.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

config-import-events-2195417-12.patch no longer applies.

error: patch failed: core/lib/Drupal/Core/Config/ConfigImporter.php:7
error: core/lib/Drupal/Core/Config/ConfigImporter.php: patch does not apply

Sutharsan’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
6.84 KB

Rerolled #12 patch

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 15: config-import-events-2195417-15.patch, failed testing.

Sutharsan’s picture

Status: Needs work » Needs review
FileSize
1.37 KB
8.08 KB

ConfigImporter changes applied to new Drupal\Core\Config\BatchConfigImporter that was added in #2004370: Batch the configuration synchronisation process.

The last submitted patch, 18: interdiff-2195417-15-18.patch, failed testing.

The last submitted patch, 18: interdiff-2195417-15-18.patch, failed testing.

tim.plunkett’s picture

Crossposting #1889474: Create tests for config event handling, that's an old major that I think is obsolete?

Gábor Hojtsy’s picture

@tim.plunkett: thanks, closed that down as duplicate of #2175917: Clean up configuration system events where tests were added :)

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

This cleanup looks good :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 65899f6 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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