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

Files: 
CommentFileSizeAuthor
#11 2261131-raw-export-11.patch3.43 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,722 pass(es). View
#8 2261131-raw-export-8-test-only.patch2.34 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,678 pass(es), 170 fail(s), and 56 exception(s). View
#8 2261131-raw-export-8.patch3.54 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,615 pass(es). View
#8 interdiff.txt2.62 KBGábor Hojtsy
#5 2261131-raw-export-5.patch2.77 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,739 pass(es). View
#5 2261131-raw-export-5-test-only.patch1.56 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,690 pass(es), 1 fail(s), and 0 exception(s). View
#4 2261131-raw-export-04.patch1.01 KBJose Reyero
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,197 pass(es). View
#1 raw-export.patch798 bytesGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,510 pass(es). View

Comments

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
798 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,510 pass(es). View

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
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,197 pass(es). View

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

Issue tags: -Novice, -Needs tests
FileSize
1.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,690 pass(es), 1 fail(s), and 0 exception(s). View
2.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,739 pass(es). View

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
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,615 pass(es). View
2.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,678 pass(es), 170 fail(s), and 56 exception(s). View

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
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,722 pass(es). View

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.