Problem/Motivation

We need a way to add steps to configuration synchronisation. We have two use cases:

Proposed resolution

  • Move the operations code from BatchConfigImporter to ConfigImporter
  • Use operation list in ConfigImporter
  • Add ability to alter the operation list

Remaining tasks

  • Agree approach
  • Write patch

User interface changes

None

API changes

Maybe none

Files: 
CommentFileSizeAuthor
#16 2238069.16.patch23.89 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,876 pass(es). View
#16 11-16-interdiff.txt5.42 KBalexpott
#12 2238069.11.patch25.5 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,691 pass(es). View
#12 5-11-interdiff.txt5.89 KBalexpott
#5 2238069.5.patch23.27 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,740 pass(es). View
#5 3-5-interdiff.txt8.8 KBalexpott
#3 2238069.3.patch22.36 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,741 pass(es). View

Comments

Gábor Hojtsy’s picture

Assigned: Unassigned » Gábor Hojtsy

Looking at this one now.

Gábor Hojtsy’s picture

Assigned: Gábor Hojtsy » alexpott

In fact alexpott is working on this.

alexpott’s picture

Status: Active » Needs review
FileSize
22.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,741 pass(es). View

The patch attached completely removes the BatchConfigImporter since once we start adding steps to the synchronisation process having a batch and non batch version.

All steps have to use the batch context array but ConfigImporter::import() still allows for a non batch implementation.

Gábor Hojtsy’s picture

Only relatively minor things, so leaving as needs review.

  1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -436,36 +450,208 @@ public function getUnprocessedExtensions($type) {
    +   * @param string|callable $operation
    +   *   The operation to call. Either a method on the ConfigImporter class or a
    +   *   callable.
    

    So in my local version of this, I made it a callable all the time for simplicity. So config importer would just return callables also in its operations generator.

  2. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -436,36 +450,208 @@ public function getUnprocessedExtensions($type) {
    +    $presync = $this->moduleHandler->invokeAll('config_import_presync_operation');
    +    $postsync = $this->moduleHandler->invokeAll('config_import_postsync_operation');
    

    operations? there may be multiple

  3. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -436,36 +450,208 @@ public function getUnprocessedExtensions($type) {
    +   * Processes extensions as a batch operation.
    +   *
    +   * @param array $context.
    +   *   The batch context.
    ...
    +   * Processes configuration as a batch operation.
    +   *
    +   * @param array $context.
    +   *   The batch context.
    ...
    +   * Finishes the batch.
    +   *
    +   * @param array $context.
    +   *   The batch context.
    
    +++ b/core/modules/config/tests/config_import_test/config_import_test.module
    @@ -4,3 +4,41 @@
    + * @param array $context
    + *   The batch context.
    ...
    + * @param array $context
    + *   The batch context.
    

    It may or may not be a batch :) It is a semi-batch... Would not be correct to say it is a batch AFAIS.

  4. +++ b/core/modules/config/tests/config_import_test/config_import_test.module
    @@ -4,3 +4,41 @@
    + * Implements pre configuration synchronization step for testing.
    ...
    + * Implements post configuration synchronization step for testing.
    

    *Operations* not step.

alexpott’s picture

FileSize
8.8 KB
23.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,740 pass(es). View

Changed the use of operations/operation to sync_steps/sync_step. This is because $op is used in the ConfigImporter to mean the operation that is currently being performed eg create field.field.some_id.

alexpott’s picture

I looked a making the method calls on the ConfigImporter use the callable syntax as well... ie. $sync_steps[] = array($this, 'processConfigurations'); this didn't work because the ConfigImporter $this gets out of sync.

Gábor Hojtsy’s picture

Is that $this lost due to batching? How is that not the same problem for anybody altering the steps then? Not the same problem there?

alexpott’s picture

@Gabor anybody adding new steps and using an object would have to use a public static method.

Gábor Hojtsy’s picture

Do we expect/advise people to use global functions then? Which sounds like the only remaining use case for callables then no? Looks like we should remove support for that then to direct people to the right thing and avoid them doing callables with instances of objects.

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -436,36 +450,206 @@ public function getUnprocessedExtensions($type) {
    +   * @param $context
    

    typehint

  2. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -436,36 +450,206 @@ public function getUnprocessedExtensions($type) {
    +      throw new ConfigImporterException(String::format('Operation %operation can not be called,', array('%operation', $sync_step)));
    

    The exception message shouldn't end in punctuation. Also, this assumes that $sync_step is a string, when it could be a malformed callable (or anything, really)

  3. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -436,36 +450,206 @@ public function getUnprocessedExtensions($type) {
    +   * @throws ConfigImporterException
    

    Here (and elsewhere), full class name please

  4. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -436,36 +450,206 @@ public function getUnprocessedExtensions($type) {
    +    $presync = $this->moduleHandler->invokeAll('config_import_presync_steps');
    +    $postsync = $this->moduleHandler->invokeAll('config_import_postsync_steps');
    +
    +    return array_merge($presync, $sync_steps, $postsync, array('finish'));
    

    These hooks need *.api.php entries.

    Also, I checked install_tasks (which this is similar to), and it includes an alter. I think allowing an alter of the merged list would be a good idea.

  5. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -436,36 +450,206 @@ public function getUnprocessedExtensions($type) {
    +   * @return array|bool
    ...
    +   * @return array|bool
    

    I think we've tried to avoid/remove these, and return empty arrays. Accidentally foreach-ing over FALSE is never fun

  6. +++ b/core/modules/config/lib/Drupal/config/Form/ConfigSync.php
    @@ -278,18 +278,20 @@ public function submitForm(array &$form, array &$form_state) {
    -  public static function processBatch(BatchConfigImporter $config_importer, $operation, &$context) {
    +  public static function processBatch(ConfigImporter $config_importer, $sync_step, &$context) {
    

    These renames are fine, but seem rather superfluous.

Addressing #9, array('Classname', 'static_method') and some_global_function would both be fine. We could try protecting against instance methods...
if (is_array($step) && is_object($step[0])) { throw new \Exception(); }
or something...

Gábor Hojtsy’s picture

@timplunkett: re 4, the idea was not to allow removal of the crucial built-in steps which is why the pre/post steps hooks were added. Otherwise the full alter hook could be the only one and we would be done :)

alexpott’s picture

FileSize
5.89 KB
25.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,691 pass(es). View

Thanks @timplunkett

  1. Fixed
  2. Changed to an InvalidArgumentException
  3. Fixed
  4. Fixed - added an alter - this could remove the crucial built in steps but but I think this is ok - if a module did this then they would have a good reason to.
  5. Out-of-scope - this was from BatchConfigImporter
  6. This is because of the other use of $operation in the ConfigImporter - we're now consistently using step
Gábor Hojtsy’s picture

All right, changes look good to me. Not entirely happy with now having 3 new hooks for this (we could just have one alter that would do the same thing?). But I don't feel strong about it. I do see the usecase of having the pre/post hooks which are safe and the alter hook which can then alter everything and knows all the *added* items are in, so we have different hooks for adding and then for possibly doing dangerous things. I don't feel strong either way.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Resolved issues raised by @timplunkett. I think we can refine this in the future as needed, but is a great first step.

Gábor Hojtsy’s picture

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
5.42 KB
23.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,876 pass(es). View

I think only the alter is necessary. If people break their site through improper API usage so be it.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Updates look good and reflect my preference from #13. I rewrote the change notice based on the changes :)

sun’s picture

Status: Reviewed & tested by the community » Needs review
  1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -436,36 +450,207 @@ public function getUnprocessedExtensions($type) {
    +        do {
    +          $this->doSyncStep($step, $context);
    +        } while ($context['finished'] < 1);
    

    Hm. We're performing all of the entity CRUD operations for all config of a single "step" ("operation"?) in one go...?

    What if the import involves to install 50 modules? (plus configuration?)

  2. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -436,36 +450,207 @@ public function getUnprocessedExtensions($type) {
    +      $context['message'] = t('Synchronising extensions: @op @name.', array('@op' => $operation['op'], '@name' => $operation['name']));
    ...
    +    $context['message'] = t('Finalising configuration synchronisation.');
    

    s/s/z/ - American English

alexpott’s picture

Status: Needs review » Reviewed & tested by the community
  1. Yes if we use the import method - but the ConfigSync form does not do this. It uses the Batch API to process each config sync operation individually.
  2. This is copied and pasted from BatchConfigImporter - I'll open a follow up to fix this and the use of t() once this is committed.

  • Commit 0a93d9a on 8.x by webchick:
    Issue #2238069 by alexpott: Create a way to add steps to configuration...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Cool beans.

The only thing I found a bit weird was that the example code referenced procedural functions vs. more "Drupal Eight-y" OO code. But Alex explained that the whole Batch API atm is highly procedural-focused and we've never gotten around to modernizing the code. There's some issue for this but I can't find it right now.

Otherwise, the code is nice and easy to read and well-tested. Committed and pushed to 8.x. Thanks!

  • Commit 59dc3c3 on 8.x by webchick:
    Revert "Issue #2238069 by alexpott: Create a way to add steps to...

  • Commit ea22b21 on 8.x by webchick:
    Issue #2238069 by alexpott, xjm: Create a way to add steps to...

Status: Fixed » Closed (fixed)

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