Problem/Motivation

The follow code in \Drupal\config\Controller\ConfigController::downloadExport() means that overrides including those from settings.php will be exported into the tarball.

    file_unmanaged_delete(file_directory_temp() . '/config.tar.gz');

    $archiver = new ArchiveTar(file_directory_temp() . '/config.tar.gz', 'gz');
    foreach (\Drupal::service('config.storage')->listAll() as $name) {
      $archiver->addString("$name.yml", Yaml::encode(\Drupal::config($name)->get()));
    }

    $request = new Request(array('file' => 'config.tar.gz'));
    return $this->fileDownloadController->download($request, 'temporary');

Proposed resolution

Fix this and add a test.

Remaining tasks

Write patch
Review

User interface changes

None

API changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
798 bytes

Can it be as simple as this one (without tests for now)?

alexpott’s picture

It can! Perhaps a comment would be nice to say why we're using getRawData instead of deactivating overrides in the ConfigFactory - the reason is we *know* we just want the raw data and we're not doing further calls to config in a chain so disabling config overrides is unnecessary overhead.

alexpott’s picture

It is tempting to also get

\Drupal::service('config.storage')->listAll()

replaced with

$this->configManager->getConfigFactory->listAll()

in this patch too. Slightly out of scope I know but could come under tidying up downloadExport() :)

Jose Reyero’s picture

FileSize
1.01 KB

Updated patch following suggestions in #3
plus, replaced too

 \Drupal::config($name) ->get()

by

$this->configManager->getConfigFactory()->get($name)

which I think is the same case.

Gábor Hojtsy’s picture

Now with test coverage :) Totally not novice AFAIS.

The patch needed a reroll so no interdiff possible. The test-only patch is the new thing, the fix just includes minor comment fixes.

The last submitted patch, 5: 2261131-raw-export-5-test-only.patch, failed testing.

alexpott’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigExportUITest.php
    @@ -73,6 +74,12 @@ function testExport() {
    +    // Ensure the test configuration override did not end up exported.
    +    $archiver->extract(file_directory_temp(), array('system.maintenance.yml'));
    +    $file_contents = file_get_contents(file_directory_temp() . '/' . 'system.maintenance.yml');
    +    $exported = Yaml::decode($file_contents);
    +    $this->assertNotIdentical($exported['message'], 'Foo');
    

    We need a positive equivalent to confirm the the value is overridden if accessed through the config factory. This will make the test more robust and less susceptible to false positives.

  2. +++ b/core/modules/config/tests/config_test/config_test.module
    @@ -10,6 +10,9 @@
    +// Override the system maintenance message for testing.
    +$GLOBALS['config']['system.maintenance']['message'] = 'Foo';
    +
    

    I'm not a huge fan of affecting all the tests that install config_test.module

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
2.62 KB
3.54 KB
2.34 KB

I agree the positive test is useful I was in fact thinking to add it myself. Done now.

I think using a separate testing module just for this one line is silly and I believe reusing existing test modules for multiple purposes is not a sin. But whatever. Created yet another test module for this one line of test code then.

Status: Needs review » Needs work

The last submitted patch, 8: 2261131-raw-export-8-test-only.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

Uploaded in the wrong order. Ready for review.

Gábor Hojtsy’s picture

FileSize
3.43 KB

Rerolled for PSR-4.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Looks great. Thanks @Gábor Hojtsy

vijaycs85’s picture

Nice +1 RTBC

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Excellent, thanks all!

Committed and pushed to 8.x. w00t!

  • Commit 086d507 on 8.x by webchick:
    Issue #2261131 by Jose Reyero, Gábor Hojtsy | alexpott: Fixed Overrides...
Gábor Hojtsy’s picture

Yay, thanks!

Status: Fixed » Closed (fixed)

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