Problem/Motivation

Configuration synchronization page is broken due to error.

Steps to reproduce

Remove the file core.extension.yml in config/sync directory.

Visit syncronise tab at: /admin/config/development/configuration

Get the below error:

TypeError: Argument 1 passed to Drupal\field\ConfigImporterFieldPurger::getFieldStoragesToPurge() must be of the type array, bool given, called in /modules/field/field.module on line 323 in Drupal\field\ConfigImporterFieldPurger::getFieldStoragesToPurge() (line 111 of core/modules/field/src/ConfigImporterFieldPurger.php).

Proposed resolution

Improve handling of empty config/sync in the field module in hook field_form_config_admin_import_form_alter.

Issue fork drupal-3385934

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

useernamee created an issue. See original summary.

useernamee’s picture

Version: 10.1.x-dev » 10.0.x-dev

useernamee’s picture

Status: Active » Needs review
cilefen’s picture

Status: Needs review » Needs work
Issue tags: -field

Version: 10.0.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

joegraduate changed the visibility of the branch 3385934-configuration-synchronization-page to hidden.

joegraduate changed the visibility of the branch 3385934-configuration-synchronization-page to active.

joegraduate’s picture

Looks like the target branch for the MR needs to be updated to to 11.x

joegraduate’s picture

Status: Needs work » Needs review
joegraduate’s picture

I cherry-picked @useernamee's commit into a new branch based on 11.x and created a new MR targeting 11.x

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new1.37 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

joegraduate’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Thanks for working on this issue.

Will need a failing test case to show the issue.

phthlaap made their first commit to this issue’s fork.

phthlaap changed the visibility of the branch 3385934-configuration-synchronization-page to hidden.

phthlaap’s picture

Status: Needs work » Needs review
joegraduate’s picture

This GitLab "Test-only changes" pipeline in MR !7103 fails as expected as of @phthlaap's latest changes:

PHPUnit 9.6.15 by Sebastian Bergmann and contributors.
Testing Drupal\Tests\field\Functional\FieldImportDeleteUninstallUiTest
.
E                                                                  2 / 2 (100%)
Time: 00:16.901, Memory: 4.00 MB
There was 1 error:
1) Drupal\Tests\field\Functional\FieldImportDeleteUninstallUiTest::testSynchronizeForm
Behat\Mink\Exception\ExpectationException: Current response status code is 500, but 200 expected.
/builds/issue/drupal-3385934/vendor/behat/mink/src/WebAssert.php:888
/builds/issue/drupal-3385934/vendor/behat/mink/src/WebAssert.php:145
/builds/issue/drupal-3385934/core/modules/field/tests/src/Functional/FieldImportDeleteUninstallUiTest.php:143
/builds/issue/drupal-3385934/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
ERRORS!
Tests: 2, Assertions: 28, Errors: 1.
joegraduate’s picture

Issue tags: -Needs tests
smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

So I tried to replicate locally by emptying my config/sync folder and going to /admin/config/development/configuration but I don't get any error or see anything in the logs.

Could additional steps be added to the issue summary.

phthlaap’s picture

Status: Needs work » Needs review

@smustgrave just remove the file core.extension.yml instead of all .yml files

smustgrave’s picture

Title and issue summary should be updated to reflect the issue.

phthlaap’s picture

Title: Configuration synchronization page errors out when config/sync is empty » The Configuration Synchronization page encounters errors when core.extension.yml is missing in the config/sync directory.
Issue summary: View changes
phthlaap’s picture

Title: The Configuration Synchronization page encounters errors when core.extension.yml is missing in the config/sync directory. » The configuration synchronization page encounters errors when core.extension.yml is missing in the config/sync directory.
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs title update, -Needs issue summary update

Title and issue summary have both been updated.

Seems like an edge case but definitely do see the issue, tests also show it

1) Drupal\Tests\field\Functional\FieldImportDeleteUninstallUiTest::testSynchronizeForm
Behat\Mink\Exception\ExpectationException: Current response status code is 500, but 200 expected.
/builds/issue/drupal-3385934/vendor/behat/mink/src/WebAssert.php:888
/builds/issue/drupal-3385934/vendor/behat/mink/src/WebAssert.php:145
/builds/issue/drupal-3385934/core/modules/field/tests/src/Functional/FieldImportDeleteUninstallUiTest.php:143
/builds/issue/drupal-3385934/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
ERRORS!
Tests: 2, Assertions: 28, Errors: 1.

Tried to see if there is a better spot to put this test but anywhere else felt like a stretch for any other test function.

Think this should be good.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I feel this fix is in the wrong place. If core.extension is missing we should have bailed way before hitting hook_config_import_steps_alter.

Given the importance of core.extension I think we can add something to \Drupal\system\SystemConfigSubscriber::onConfigImporterValidateNotEmpty like

    if (!isset($importList['core.extension')) {
      $event->getConfigImporter()->logError($this->t('This import does not contain core.extension configuration, so has been rejected.'));
    }

Eventually schema validation is going to do this for us.

pradhumanjain2311 made their first commit to this issue’s fork.

phthlaap’s picture

StatusFileSize
new82.32 KB

#27 It will impact the import configuration form when there are no changes in the core extension.

alexpott’s picture

Ahh interesting... must have forgotten something along the way... posted a formulation on the MR that will work...

Keshav Patel made their first commit to this issue’s fork.

phthlaap’s picture

@alexpott the core.extension configuration is validated in core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php:93. I think we no need to validate again.

    $config_importer = $event->getConfigImporter();
    if ($config_importer->getStorageComparer()->getSourceStorage()->exists('core.extension')) {
      $this->validateModules($config_importer);
      $this->validateThemes($config_importer);
      $this->validateDependencies($config_importer);
    }
    else {
      $config_importer->logError($this->t('The core.extension configuration does not exist.'));
    }

It also have tests to cover: core/tests/Drupal/KernelTests/Core/Config/ConfigImporterTest.php::testMissingCoreExtension

But even when it is validated I think we also need fix the core/modules/field/field.module:324 to prevent the TypeError message when access the page /admin/config/development/configuration

phthlaap’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Re-ran test-only feature https://git.drupalcode.org/issue/drupal-3385934/-/jobs/1326634 which shows the test is still covering the change.

Believe the feedback from @alexpott has been addressed

I did save credit for the users who have worked on the issue, did not include just applying a suggestion will let committers decide that.

  • larowlan committed 54096101 on 10.3.x
    Issue #3385934 by phthlaap, joegraduate, useernamee, alexpott: The...

  • larowlan committed 563e9a68 on 10.4.x
    Issue #3385934 by phthlaap, joegraduate, useernamee, alexpott: The...

  • larowlan committed 3a3973ed on 11.0.x
    Issue #3385934 by phthlaap, joegraduate, useernamee, alexpott: The...

  • larowlan committed 96a211c7 on 11.x
    Issue #3385934 by phthlaap, joegraduate, useernamee, alexpott: The...
larowlan’s picture

Version: 11.x-dev » 10.3.x-dev
Issue tags: +Bug Smash Initiative

Committed to 11.x and backported to 11.0.x, 10.4.x and 10.3.x

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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