Since we now delegate to the config entity storage controller there no longer is any need to create manifest entries in config_install_default_config().

CommentFileSizeAuthor
#9 4-9-interdiff.txt459 bytesalexpott
#9 1889752.9.drupal8.manifest.patch5.63 KBalexpott
#4 3-4-interdiff.txt1.07 KBalexpott
#4 1889752.4.drupal8.manifest.patch5.02 KBalexpott
#3 1-3-interdiff.txt3.98 KBalexpott
#3 1889752.3.drupal8.manifest.patch4.8 KBalexpott
#1 1889752.drupal8.manifest.patch2.61 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Active » Needs review
FileSize
1889752.drupal8.manifest.patch2.61 KB

Here's a patch that adds a test to ensure manifests are written for config entities and removes the unnecessary code - which was made unnecessary due to #1806178: Remove code duplication for hook_config_import_*() implementations of config entities.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented

I think we always want to have a manifest file even when it is empty, and right now there will be no manifest if a module is installed with no default config entities out of the box. So I'd like to see an empty manifest file created in config_install_default_config() for consistency's sake. Otherwise this is a great changed, that original code was pretty ugly, I'm glad to see it go.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
1889752.3.drupal8.manifest.patch4.8 KB
1-3-interdiff.txt3.98 KB

New patch that ensures an empty manifest is created for each config entity whether or not a default config entity is provided. Tests added for this too.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
1889752.4.drupal8.manifest.patch5.02 KB
3-4-interdiff.txt1.07 KB

Actually we want to create empty manifest files even if the module has no config directory!

Status: Needs review » Needs work
Issue tags: -Configuration system, -Configurables

The last submitted patch, 1889752.4.drupal8.manifest.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Needs review
Issue tags: +Configuration system, +Configurables

#4: 1889752.4.drupal8.manifest.patch queued for re-testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

Got a fatal php error in testRegression_310447() - nought to do with this change - so requested retest.

Status: Needs review » Needs work

The last submitted patch, 1889752.4.drupal8.manifest.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Needs review
FileSize
1889752.9.drupal8.manifest.patch5.63 KB
4-9-interdiff.txt459 bytes

oh wow! the fatal is caused by this patch ...

exception 'PDOException' with message 'SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal8alt.simpletest694268node_type' doesn't exist' in /Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Database/Statement.php:58
Stack trace:
#0 /Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Database/Statement.php(58): PDOStatement->execute(Array)
#1 /Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Database/Connection.php(523): Drupal\Core\Database\Statement->execute(Array, Array)
#2 /Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Database/Query/Select.php(421): Drupal\Core\Database\Connection->query('SELECT nt.*?FRO...', Array, Array)
#3 /Users/alex/dev/sites/drupal8alt.dev/core/modules/node/node.module(725): Drupal\Core\Database\Query\Select->execute()
#4 /Users/alex/dev/sites/drupal8alt.dev/core/modules/node/node.module(370): _node_types_build()
#5 /Users/alex/dev/sites/drupal8alt.dev/core/modules/node/node.module(202): node_type_get_names()
#6 /Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Plugin/Discovery/InfoHookDecorator.php(59): node_entity_info(Array)
#7 /Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Component/Plugin/Discovery/ProcessDecorator.php(63): Drupal\Core\Plugin\Discovery\InfoHookDecorator->getDefinitions()
#8 /Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Plugin/Discovery/AlterDecorator.php(58): Drupal\Component\Plugin\Discovery\ProcessDecorator->getDefinitions()
#9 /Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Component/Plugin/PluginManagerBase.php(59): Drupal\Core\Plugin\Discovery\AlterDecorator->getDefinitions()
#10 /Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Entity/EntityManager.php(264): Drupal\Component\Plugin\PluginManagerBase->getDefinitions()
#11 /Users/alex/dev/sites/drupal8alt.dev/core/includes/entity.inc(36): Drupal\Core\Entity\EntityManager->getDefinitions()
#12 /Users/alex/dev/sites/drupal8alt.dev/core/includes/config.inc(292): entity_get_info()
#13 /Users/alex/dev/sites/drupal8alt.dev/core/includes/config.inc(29): config_get_module_config_entities('database_test')
#14 /Users/alex/dev/sites/drupal8alt.dev/core/includes/module.inc(549): config_install_default_config('module', 'database_test')
#15 /Users/alex/dev/sites/drupal8alt.dev/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php(223): module_enable(Array, false)
#16 /Users/alex/dev/sites/drupal8alt.dev/core/modules/system/lib/Drupal/system/Tests/Database/DatabaseTestBase.php(22): Drupal\simpletest\DrupalUnitTestBase->enableModules(Array)
#17 /Users/alex/dev/sites/drupal8alt.dev/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php(702): Drupal\system\Tests\Database\DatabaseTestBase->setUp()
#18 /Users/alex/dev/sites/drupal8alt.dev/core/scripts/run-tests.sh(381): Drupal\simpletest\TestBase->run()
#19 /Users/alex/dev/sites/drupal8alt.dev/core/scripts/run-tests.sh(22): simpletest_script_run_one_test('17', 'Drupal\system\T...')
#20 {main}

So when installing the 'database_test' module simpletest thinks it's got node installed but actually it hasn't... funnily enough this test does not even need node installed.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

#9: 1889752.9.drupal8.manifest.patch queued for re-testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

Thanks!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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