I count two serious bugs and an architecture mishap and a ton of minors. I am not following this issue.

foreach (config_get_module_config_entities($name) as $entity_type => $entity_info) {

$entity_type is not used. Minor

  foreach (config_get_module_config_entities($name) as $entity_type) {
    config('manifest.' . $entity_info['config_prefix'])->delete();
  }

Now, this one is missing the entity_info. Where's the test for this?

foreach (array_diff_assoc($target_config_data, $source_config_data) as $name => $value) {

$name is not used

catch (ConfigException $e) {

misses use
config_import_invoke_owner doesnt have t@return doxygen.

module_invoke($module, 'config_import_' . $op, $name, $new_config, $old_config);

There is absolutely no way to get to the $source_storage as both config objects are carrying the $target_storage.

return ($entity_info['module'] == $module) && is_subclass_of($entity_info['class'], 'Drupal\Core\Config\Entity\ConfigEntityInterface');

pointless (and slow) to use is_subclass_of instead of the instanceof operator.

Files: 
CommentFileSizeAuthor
#12 drupal-1843964-11.patch1.72 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 50,407 pass(es), 1 fail(s), and 0 exception(s). View
#5 bad_config_bad-1843964-5.patch2.62 KBsuperspring
None View

Comments

YesCT’s picture

Issue tags: +config.inc

The actionable thing to do is identify each separate problem and file an issue for each one. In case someone does this.. they might find patching dreditor with the clone issue patch helpful. #1803622: Add a create follow-up issue link which fills in values (clone an issue)

chx’s picture

Issue tags: +Novice

Splitting this out is a novice task (which i lacked the time for). Actually fixing most is also a novice.

nagwani’s picture

would like to help with this issue. Will post a patch soon.

Damien Tournoud’s picture

return ($entity_info['module'] == $module) && is_subclass_of($entity_info['class'], 'Drupal\Core\Config\Entity\ConfigEntityInterface');

pointless (and slow) to use is_subclass_of instead of the instanceof operator.

This one looks legit. $entity_info['class'] is a class name as a string, not an object. instanceof only works on classes and instantiated objects.

superspring’s picture

Status: Active » Needs review
FileSize
2.62 KB
None View

This patch addresses the issues above excluding:

<?php
module_invoke($module, 'config_import_' . $op, $name, $new_config, $old_config);
?>

Since the data from source_storage is loaded into the new config, so the source config is still available.

<?php
return ($entity_info['module'] == $module) && is_subclass_of($entity_info['class'], 'Drupal\Core\Config\Entity\ConfigEntityInterface');
?>

...and this code because I cannot see an object to use instanceof against.

chx’s picture

Status: Needs review » Needs work

The use Drupal\Core\Config\ConfigException; and the entity_info changes definitely points to missing tests. As a bandaid, add source_storage to the module_invoke command as an extra argument and the API docs.

chx’s picture

Issue tags: -Novice +Needs tests

And thanks for the work on this!

nagwani’s picture

opps, sorry was out on vacation, back now.

sun’s picture

Issue tags: +Configuration system
heyrocker’s picture

Title: config.inc is a mess » config.inc cleanups

Giving more productive title so to prevent me from wanting to throw my computer every time I see it in my tracker.

alexpott’s picture

Status: Needs work » Needs review
+++ b/core/includes/config.incundefined
@@ -72,7 +73,7 @@ function config_uninstall_default_config($type, $name) {
   // If this module defines any ConfigEntity types, then delete the manifest
   // file for each of them.
-  foreach (config_get_module_config_entities($name) as $entity_type) {
+  foreach (config_get_module_config_entities($name) as $entity_info) {
     config('manifest.' . $entity_info['config_prefix'])->delete();
   }

This is more problematic than just the code. At this point the module is disabled and so config_get_module_config_entities($name) is never going to return anything. See #1831776: removing manifest files from uninstalled modules

dawehner’s picture

FileSize
1.72 KB
FAILED: [[SimpleTest]]: [MySQL] 50,407 pass(es), 1 fail(s), and 0 exception(s). View

Just a rerole, no actual tests.

Status: Needs review » Needs work

The last submitted patch, drupal-1843964-11.patch, failed testing.

alexpott’s picture

Status: Needs work » Closed (won't fix)

None of this code exists anymore :)