config.inc cleanups [#1843964]

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.

Comment	File	Size	Author
#12	drupal-1843964-11.patch	1.72 KB	dawehner
#12
#5	bad_config_bad-1843964-5.patch	2.62 KB	superspring
#5

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

YesCT CreditAttribution: YesCT commented 19 November 2012 at 06:20

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)

Comment #2

chx CreditAttribution: chx commented 19 November 2012 at 14:28

Issue tags:

+Novice

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

Comment #3

nagwani CreditAttribution: nagwani commented 20 November 2012 at 06:30

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

Comment #4

Damien Tournoud CreditAttribution: Damien Tournoud commented 20 November 2012 at 08:06

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.

Comment #5

superspring CreditAttribution: superspring commented 22 November 2012 at 23:22

Status:

Active

» Needs review

File	Size
bad_config_bad-1843964-5.patch	2.62 KB

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.

Comment #6

chx CreditAttribution: chx commented 23 November 2012 at 02:37

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.

Comment #7

chx CreditAttribution: chx commented 23 November 2012 at 02:37

Issue tags:

-Novice

+Needs tests

And thanks for the work on this!

Comment #8

nagwani CreditAttribution: nagwani commented 24 November 2012 at 06:44

opps, sorry was out on vacation, back now.

Comment #9

sun

German

Karlsruhe

CreditAttribution: sun commented 30 January 2013 at 19:59

Issue tags:

+Configuration system

Comment #10

gdd

he/him

English

Portland, OR

CreditAttribution: gdd commented 13 February 2013 at 20:34

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.

Comment #11

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 13 February 2013 at 21:27

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