Problem/Motivation

(Why the issue was filed, steps to reproduce the problem, etc.)

Proposed resolution

When you have the config rewrite file for the configuration that might exist in your system, but it is not, config rewrite creates the new configuration with the settings only from your changes and that cause a lot of problems with invalid configuration.

This method from configFactory class:

$config = $this->configFactory->getEditable($file->name);

returns the config object always, even if config is not present.

This causes original_data from here

$original_data = $config->getRawData();

to be empty and after merging it with rewrite it gives the same rewrite array as you have in your rewrite yml file.

I propose to check if the original_rewrite array, read from yml file, is equal to the array after rewriteConfig method. This will allow to avoid the issue with corrupted configuration.

Remaining tasks

  1. Review the patch
  2. Write tests
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

a.dmitriiev created an issue. See original summary.

brantwynn’s picture

Issue summary: View changes
Status: Active » Needs work

This looks like a good solution. Can we get an additional test added?

brantwynn’s picture

Here's an updated patch that should address the issue. I've included a bit more logic for checking if the rewrite should be saved in addition to some more logging and added a step to the test that should pass...

brantwynn’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 3: allow_optional_config-2903629-3.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

brantwynn’s picture

Status: Needs work » Needs review
FileSize
6.91 KB

This is an updated patch that changes a few minor things and accomodates the failing multilingual test.

We had been creating translated config overrides on the fly here too but now that existing translation overrides do not yet exist, the check shows they fail during import. Therefore role.test4 will import as its original version, but config rewrite does not allow for the translation overrides to be written. We actually do want that feature though, so logic had to be changed to accomodate it.

In any case, this should allow for not-yet-existing overrides to be created (yeah, they'll load an empty $original_data object but thats ok).

At the same time, the new logic should catch when we're dealing with a standard config rewrite being attempted on an empty config object (which is the use case described that we need to avoid in the OP).

Status: Needs review » Needs work

The last submitted patch, 6: allow_optional_config-2903629-6.patch, failed testing. View results

brantwynn’s picture

Status: Needs work » Needs review
FileSize
6.79 KB

My bad, left some comments in that should have been removed to pass the phplint.

brantwynn’s picture

@a.dmitriiev I am going to leave this as-is over the weekend and look to commit it next week if no objections.

a.dmitriiev’s picture

Hi @brantwynn,

Thanks, for taking care about this issue. I will try to test the new patch, but from the quick view it seems to do what it is supposed to do. I will test and write my feedback in the issue.

brantwynn’s picture

Title: Allow optional config rewrite » Stop creating config during rewrites of optional or non existing config
brantwynn’s picture

Updated patch with a minor fix that adds the missing @langcode.

// Log a message indicating whether the config rewrite was saved or not.
          $log = $langcode ? '@config (@langcode) @result by @module' : '@config @result by @module';
          $this->logger->notice($log, [
            '@config' => $file->name,
            '@result' => $result,
            '@module' => $extension->getName(),
            '@langcode' => $langcode
          ]);
brantwynn’s picture

Added some better logging for when $langcode is being used.

samuel.mortenson’s picture

This updated patch adds support for optional config rewrites as a part of a profile install, which is a tricky problem as optional config is actually installed after all dependencies of an install profile are installed.

This change requires #2924549: Invoke hook after a site install is complete but should not affect the existing behavior of config rewrite if you do not patch core. All install profiles using config rewrite to rewrite optional config will need to patch core, after this is committed.