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 |
#5 | bad_config_bad-1843964-5.patch | 2.62 KB | superspring |
Comments
Comment #1
YesCT CreditAttribution: YesCT commentedThe 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 commentedSplitting this out is a novice task (which i lacked the time for). Actually fixing most is also a novice.
Comment #3
nagwani CreditAttribution: nagwani commentedwould like to help with this issue. Will post a patch soon.
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis 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 commentedThis patch addresses the issues above excluding:
Since the data from source_storage is loaded into the new config, so the source config is still available.
...and this code because I cannot see an object to use instanceof against.
Comment #6
chx CreditAttribution: chx commentedThe 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 commentedAnd thanks for the work on this!
Comment #8
nagwani CreditAttribution: nagwani commentedopps, sorry was out on vacation, back now.
Comment #9
sunComment #10
gddGiving more productive title so to prevent me from wanting to throw my computer every time I see it in my tracker.
Comment #11
alexpottThis 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
Comment #12
dawehnerJust a rerole, no actual tests.
Comment #14
alexpottNone of this code exists anymore :)