Since image module is already in core, and #1447686: Allow importing and synchronizing configuration when files are updated is looking closer to getting in, I'm opening this as critical - I don't think we can release without resolving this problem, and it already firmly exists in core.

There are at least four ways that an image style can end up added or updated in CMI;

1. image_style_save():

http://api.drupal.org/api/drupal/core%21modules%21image%21image.module/f...

These two lines are the important ones:

 // Let other modules update as necessary on save.
  module_invoke_all('image_style_save', $style);

  // Clear all caches and flush.
  image_style_flush($style);

2. Image module ships with three default image styles. However, these aren't saved via image_style_save() when the module is enabled, instead they're saved by config_install_default_config(), which does not invoke any hooks:

http://api.drupal.org/api/drupal/core!includes!config.inc/function/confi...

Similarly hook_image_style_delete() won't be invoked if a module providing image styles is removed on uninstall.

3. Once #1447686: Allow importing and synchronizing configuration when files are updated lands, image styles will also be saved if they're part of new or updated configuration in files, this required the addition of this code, which is mostly copied from image_style_save(), this is more or less straight code duplication, that runs the risk of getting out of sync:

 /**
+ * Implements hook_config_sync().
+ */
+function image_config_sync($config_changes, $target_storage, $source_storage) {
+  foreach ($config_changes['new'] as $file_name) {
+    if (strpos($file_name, 'image.style.') === 0) {
+      $style = $source_storage->load($file_name)->get();
+      $style['is_new'] = TRUE;
+      module_invoke_all('image_style_save', $style);
+      image_style_flush($style);
+    }
+  }
+  foreach ($config_changes['changed'] as $file_name) {
+    if (strpos($file_name, 'image.style.') === 0) {
+      $style = $source_storage->load($file_name)->get();
+      $style['is_new'] = FALSE;
+      module_invoke_all('image_style_save', $style);
+      image_style_flush($style);
+    }
+  }
+  foreach ($config_changes['deleted'] as $file_name) {
+    if (strpos($file_name, 'image.style.') === 0) {
+      // The style has been deleted, so read the previous configuration from the
+      // old storage.
+      $style = $target_storage->load($file_name)->get();
+      image_style_flush($style);
+
+      // @todo image_style_delete() supports the notion of a "replacement style"
+      //   to be used by other modules instead of the deleted style. Good idea.
+      //   But squeezing that into a "delete" operation is the worst idea ever.
+      //   Regardless of Image module insanity, add a 'replaced' stack to
+      //   config_sync()? And how can that work? If an 'old_ID' key would be a
+      //   standard, wouldn't this belong into 'changed' instead?
+      $style['old_name'] = $style['name'];
+      $style['name'] = '';
+      module_invoke_all('image_style_delete', $style);
+    }
+  }
+}

4. Someone could potentially save a new/updated image style using config() directly, bypassing image_style_save().

5. We'll likely have a 'reset to defaults' button at some point, unless that uses the exact same code as the module defaults stuff, it'll be yet another way for this to happen.

So for the moment, that's four ways to create/update image styles, only two of which fire hooks. The result of this issue might be we say "don't do that" in terms of directly saving to config, but that still leaves the module install/uninstall issue.

Proposed solution

When configuration is written to the active store, regardless of the action that triggers this, the same hooks should be consistently fired. There might be one or two cases where we want to add an extra hook for a specific event (like sycing from files or whatever), but that should be added to a common base, not a completely different code path.

Remaining tasks

Figure out how to get consistent hook invocation, when there is supposed to be a 'dumb' configuration API that doesn't depend on the module system.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Issue tags: +Configuration system

Note: In practical terms, this duplicates the discussion on #1552396: Convert vocabularies into configuration to some extent. Perhaps it's good to split it out, but I'm going to mark this issue as duplicate as soon as we start to literally duplicate the discussion. ;)

Either way, to resolve the situation, I'm currently playing with the following idea:

  1. Convert image styles also into "typed" configuration (as being proposed for vocabularies over there).
  2. Inject a class: Drupal\image\ImageStyle property; either into each image style config object, or into a general config object container that holds the info for all typed config.
  3. Introduce the ConfigObjectBase class as proposed in the vocabularies patch, and make the config system instantiate and return that object instead of the plain config object. (this is sorta in line with some other internal refactoring we want to do; splitting the ConfigObject off from DrupalConfig)
  4. Lastly, for such typed config, require the module system to be bootstrapped, so hooks can always be invoked, and it's not possible to save an e.g. image style config object without it.
catch’s picture

I don't think this is a duplicate at all, this is an existing critical bug in core, that issue is trying to convert a new subsystem to configuration. The issue title/component etc. of the vocabularies issue presents absolutely no clue that it might also fix image styles, nor should image style conversion be re-done in a patch for taxonomy module. So if anything we should postpone that issue on this one if they have to fix the same stuff.

It sounds like what you're essentially proposing is a two-class system for configuration.

1. Dumb configuration (equivalent to varable_get()/variable_set()) that doesn't allow anything else to interact with it.

2. Higher-level configuration objects that have an explicit dependency on the hook system when being saved/updated (what about retrieved then?), for which a straight $config->save() will no longer be valid.

That's a large architectural change compared to what we have now, but short of invoking hooks for all configuration changes, which is not really desired, I don't see an obvious way around it.

sun’s picture

Yes, "a two-class system for configuration" is the essence of what I'm proposing and recommending.

From a very high level perspective, this turns some config objects into "entities" representing configuration objects. The goal would be to ensure a consistent DX between entities and configuration, but without making "typed" config objects actual entities (because they are not and have a couple of fundamental differences), as discussed in #1552396: Convert vocabularies into configuration.

Overall, this is also in line with the vision of abstracting meta data from Field API not only for entity-level properties in Entity API, but also to an extent that it works for configuration object properties, as discussed in #1498270: Meta data for configuration.

It is also in line with all the other configuration system re-architecture tasks being outlined on #1560060: [meta] Configuration system roadmap and architecture clean-up -- most notably #1605324: Configuration system cleanup and rearchitecture, which is going to introduce a ConfigObject that is separated from DrupalConfig (the high-level storage manager/arbitraror, which mainly decides what stores are appropriate to use) and individual storage controllers (which perform read/write/delete operations only). Thus, instead of returning and acting on a DrupalConfig object, runtime code will act on a ConfigObject that is specific for each configuration object in the future.

In turn, "typed" config objects, as outlined in #1, would most probably be objects based on a class that extends the plain ConfigObject, and which contains the additional logic for invoking hooks (and whatever else may be required).

(But note again, yes, this "reminds" a lot of entities and Entity API, but I currently do not see why it would make sense to turn those typed config objects into "pseudo-entities", given that there are a couple of fundamental differences.)

And yes, for typed/classed config objects, we can discuss to re-introduce load and alter hooks. (Though that said, @fago just mentioned in another issue that he'd actually like to remove those from Entity API, and require entities to be saved instead.)


The only question that is still unclear for me is whether the class to use for a config object should be (or can be) defined and be custom per config object (i.e., a top-level class property), as opposed to a "global" definition (state/config) that applies to all config objects of a certain type (which in itself begs the question of how to identify the config object "type").

The former would basically mean this flow in the long term:

$config = config('image.style.foo');
    ---------|
    |
    v
  $data = DrupalConfig::load('image.style.foo')
    $raw = DatabaseStorage::read('image.style.foo')
      DatabaseStorage::decode($raw)
  return new DrupalConfig::getObject(isset($data['class']) ? $data['class'] : 'ConfigObject', $data)

For the latter (global definition), I'm not sure how it would look.

catch’s picture

Title: Image style saving (and etc.) sometimes fires hooks, sometimes doesn't » Config saving is completely inconsistent in terms of hook invocation and etc.
chx’s picture

Status: Active » Needs review
FileSize
2.17 KB

Now we have hook consistency. Also, we have so many hooks it comes out of our ears :P Obviously documentation will be required but first, let's see whether this passes the testbot and the community muster.

chx’s picture

FileSize
2.17 KB

Meh!

chx’s picture

Note: this solves the problem when you want to do something on new config creation (say, create field sql tables) because $this->read() will conveniently be empty.

Status: Needs review » Needs work

The last submitted patch, 1609760_6.patch, failed testing.

chx’s picture

FileSize
2.18 KB

Sorry bot.

chx’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1609760_9.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review

i don't like the 'typed config' approach.

i don't think we should say 'if your config file doesn't represent a "typed thing" (or something..), then no chance to respond for you!". i think this is too narrow - the config system should provide a consistent method to run code in response to any config object being saved.

also, i don't see whey we need the cognitive overhead of 'typed config' to allow for arbitrary code to run in response to config('foo.bar')->save().

Anonymous’s picture

Status: Needs review » Needs work

woops. bot cross posted on me.

catch’s picture

Just invoking a hook all the time seems a lot simpler to me as well.

The only issue is it means a hard dependency on the module/hook system when saving configuration to the active store, and we need to think carefully about whether that's a problem or not.

chx’s picture

Status: Needs work » Needs review
FileSize
4.86 KB

This should pass.

Edit re #14 one, are we going to save before full bootstrap? That sounds a little crazy to me. However, we could probably assign a namespace (bootstrap?) where the presave is bypassed and woe unto you if you save invalid config cos the system wont be able to hold your hand unlike with everything else.

catch’s picture

I don't think we're likely to save before full bootstrap. We've supported variable_set() before full bootstrap but that's because it's been mis-used for things that won't get migrated to CMI.

The only use case would likely be a drush vget/vset equivalent but that doesn't particularly need to avoid full bootstrap that I can think of, at least in the set case.

However, if we want to inject dependencies into the config system, we'd need to have the module system to inject, but we won't before full bootstrap - even if it's needed for the save() operation. I think that's likely the reason it's been kept out of the base API so far.

There's also #1608842: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config though. If we get to the point where we can save the first module using the config API properly, then that's going to invoke hooks, but there won't be any modules installed yet. However in that case possibly the config call to get the list of modules is just going to be empty, so possibly even this case is fine but it could start getting very tricky around that point. However, there's hook_modules_enable/disable/installed/uninstalled() which are the exact use case we're trying to deal with (reacting to changes in configuration), so it's a problem that's going to be run into anyway.

sun’s picture

The reason this has been kept out is that the config system should fundamentally be its own, self-contained component as much as possible.

That has been a top priority since the beginning. In all architectural discussions, we intentionally left complex business logic to API users/implementations. The configuration system is an API to lookup, load, save, and delete configuration objects from 1-N stores. That, and only that.

Therefore, I consider the proposed approach as fundamentally wrong:

  1. It mixes high-level Drupalisms into the low-level config system component.
  2. It adds a dependency on the module/hook system for all configuration, regardless of whether particular configuration actually needs/wants to allow modules/hooks to react or not.
  3. It injects the entire module/hook system insanity into the configuration system, which suddenly needs to cater for all the special cases, for every piece of configuration it operates on.
  4. It is inconsistent in not providing load + alter hooks for the load case, but such hooks cannot be provided for all config. If at all, it's only valid for high-level config that requires module code (and other modules) to exist in the first place.
  5. Recipe for chicken+egg disaster: module/hook system is (or will be) dependent on config system. This proposal adds the reverse dependency.

Contrary to that, the approach I outlined in #1 + #3 keeps all the high-level module layer implementation and business logic out of the config system itself by extending it to allow certain config objects to be based on an enhanced config object class, specialized for a certain type of config. Essentially, it is adding support for an optional CRUD controller class for config objects. That approach is in no way limited or specialized to support the module/hook system only, so it not only works for high-level config objects like image styles and taxonomy vocabularies, but instead, it also has the potential to do interesting things for other config objects that might benefit from built-in CRUD behavior.

Anonymous’s picture

"Contrary to that, the approach I outlined in #1 + #3 keeps all the high-level module layer implementation and business logic out of the config system itself by extending it to allow certain config objects to be based on an enhanced config object class, specialized for a certain type of config."

i don't see the difference here in terms of avoiding the dependency on the module system. those 'high level' config objects are still config objects.

trying to step back a bit - the fundamental issue in my mind is that we need arbitrary, non-config system code to run on ->save() and ->delete(). i think invoking a hook is the simplest way to do that, and i think trying to keep all aspects of the config system free from the 'insanity' of module hooks is a mistake.

"Recipe for chicken+egg disaster: module/hook system is (or will be) dependent on config system. This proposal adds the reverse dependency."

wat? no one is suggesting that the module system be required to read config, so i have no idea what the problem is here. we read the egg during early bootstrap, and write the chicken with a fully bootstrapped drupal ;-)

sun’s picture

What I'm saying is:

  1. Your module/hook system configuration is in a broken state, for whatever reason.
  2. You cannot write a fixed/corrected configuration for it, because saving it invokes the module/hook system.

Yes, taking a step back is the proper step. From a general standpoint, the config system should not deal with anything of this.

The configuration system is the equivalent of variable_get()/variable_set() for variables and db_select()/db_merge()/db_delete() for high-level config that lives in database tables.

Were you able to fiddle with configuration without using the proper module API functions (that are invoking hooks)? Yes, you were.

config('foo')->save() does and should do exactly the same. It's just that we now also want to introduce a mechanism for synchronizing configuration between stores, which does not know whether a particular config object belongs to a certain module's API or not.

#1552396: Convert vocabularies into configuration attempted to resolve this at least from one direction - i.e., when being initiated from the module API, there'd be a "wrapper" object around the actual config object that handles the necessary extra business logic for loading/saving/deletion.

This issue here wants to find a solution for the opposite direction - i.e., when being initiated from the config system, and only given the config data itself, find the "path" to the module API - if any - so the necessary extra business logic for a particular config object can be executed.

Where we disagree is that the introduction of a synchronization mechanism immediately requires us to make all config aware of and dependent on the module/hook system.

So, another possible approach would be to change the config_sync() design to act on individual config objects, instead of entire stacks of new/changed/deleted config objects. For each config object, try to find the matching module API function:

$found = FALSE;
$parts = explode('.', $name);
do {
  $function = implode('_', $parts) . '_save';
  $found = function_exists($function);
} while (!$found && array_pop($parts));

And if any, use that instead of the pure/plain config save operation:

if ($found) {
  $function($config);
}
else {
  $target_storage->write($data);
}

However, that's really only the procedural equivalent (involving magic API function names) of the declarative approach in #1 + #3 (and contrary to that, it would still have to be hard-coded into config system).

That being said, what I really don't like about the proposed built-in hooks patch here is that it also introduces magic hook names, which:

  • entirely depend on the config object name only (so you can't change config object names without changing hook function names)
  • enforces all hooks to take exactly one argument (the config object; regardless of whether a module's API might need more)
  • will inherently lead to hook name/function name clashes in contrib (due to the magic conversion of config object names into hook names; i.e., image.style.foo == image_style.foo).
Anonymous’s picture

"That being said, what I really don't like about the proposed built-in hooks patch here is that it also introduces magic hook names, which..."

yeah, i don't like that about this patch either. i mentioned it to chx - if we do go the hook invoke route, i think we should just invoke a single, known hook name, like config_presave or whatever.

will have a think about what you've proposed in #19. i'm interested in other people's opinions as well at this point.

chx’s picture

The image style configuration is called image.style.' . $style['name'] -- you cant make a singular function out of this easily centrally because you would need to figure out from image.style.large that you want image_style_config. Impossible. Of course, we could do more or less but I do not see the namespace collissions a big problem to be honest. Yes it's somewhat magic function naming but , already , calling a module image_style is calling for trouble. This is a rehash of module naming conventions, again (and again).

chx’s picture

Assigned: Unassigned » chx

Oh! I re-read sun's proposal of using a singular function -- edit: he suggests that to handle saving and I say no, it should be instead of the presave hook I proposed. I think I can live with that, yes. It's noone's business but the saving module's to act on it. I will code it tomorrow.

The reason for this is that the target writing should always happen if the presave didnt throw an exception so why mandate it for every module having additional logic to copy-paste that? It would lead to chaos if all sorts of data storage would mushroom instead of the centralized one. So: let's keep the centralized write and run that single function in a presave fashion.

sun’s picture

Assigned: chx » Unassigned

The singular API function alternative has the very same magic naming problems as the hook proposal, as outlined in #19. I consider the hard binding between config object names and API function (or hook) names as bad design.

A global, catch-all hook_config_save() and hook_config_delete() hook wouldn't have the magic naming problem, yes, but it also means that we'd possibly invoke hundreds of hook implementations for every single save or delete operation, and every single hook implementation would have to perform a super ugly string comparison check before doing anything, pretty much like the currently suggested hook_config_sync() implementation being visible in the summary; i.e., this part:

function image_config_save($config) {
  if (strpos($config->getName(), 'image.style.') === 0) {
    image_style_save($config->get());

Given all that, I'd really love to show you guys how stupidly simple and clean the implementation of #1 + #3 would look like, but that requires a ConfigObject, and I do not want to duplicate the work of #1605324: Configuration system cleanup and rearchitecture any further. @beejeebus started to mock some gist code for that a couple of days ago already — I'm eagerly waiting for a branch or patch for that issue to move forward.

chx’s picture

No I do not want a single catch-all hook either. That'd be problematic and ugly.

On the other hand, I do not consider the hard binding between config object names and an API function a bad design. To the contrary. Also I do not like the type config introduced over at the vocabulary issue either. I will see whether I can come up with a simpler patch for that there.

chx’s picture

FileSize
10.59 KB

This patch converts the dots to two underscores (merlinofchaos' idea, very good). No chance of collisions. Adds doxygen. Adds an is_new argument to the hook family so it works if the original config was an empty array (weird edge case). Skips the extra read() query on save(). Throws an exception if you are trying to save without bootstrapped. Reach into the storage directly if you really want to bypass the hooks. I know it installs (because I fixed an ill-placed config save which shown my exception was , indeed , working) and I hope somewhat it still passes as well.

chx’s picture

FileSize
10.58 KB

This one fixes the is_new / isNew property mixup I had in #25.

Status: Needs review » Needs work

The last submitted patch, 1609760_26.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
10.65 KB

The name of hooks start with config_presave__ not with config_presave_ .

chx’s picture

FileSize
10.52 KB

Now with much less debug, I found even #25-#26 had debug in it.

sun’s picture

It looks like we need to and should do #1605324: Configuration system cleanup and rearchitecture first, since that (absolutely required) architectural change seems to be the cause for a range of misunderstandings.

In light but also regardless of that:

Look.

What you're saying is:

  • Let's just implant some hooks and magic hook names, and just hope that everyone and everything will be able to work with that.

What I'm saying is:

  • If we're going to add that full module/hook system dependency in the first place, then why do we want to shoehorn ourselves or anyone else into anything particular in the first place?
  • Why don't we allow individual APIs to handle their config objects in the way that makes most sense to them?
  • Why do we want to prevent and disallow special and custom CUD behaviors — even though we know that all the stuff we're converting to this pattern was previously (or is currently) backed by module API functions that do all kind of crazy things before even attempting to save or delete?
  • With the changes of above mentioned issue, code in the module/implementation layer will not receive and generally not act on the DrupalConfig anymore, but on a (default) ConfigObject instead (which is actually unique to the $name being passed in; the DrupalConfig and storage controllers are always the same, and we stop the braindead insanity of instantiating new DrupalConfig + storage controller class objects for every single config object name we need).
  • And so, given that, in OO, it's only a "shift of a bit" to conditionally instantiate WhateverConfigObject that extends ConfigObject, instead of ConfigObject itself. And that class can handle all the extra business logic that may be required for a certain thing.
  • Lastly, the custom config object class algorithm can happily be all-optional — if it's not available/loadable for some reason, then the config system can always and easily fall back to its default implementation, which guarantees that a (bare) config object can always be operated on, regardless of current bootstrap state or availability of subsystems.
  • So why don't we retain total sanity and flexibility?
chx’s picture

I am not sure what to say to this. You suggest totally refactoring the system we have. I suggest adding a (family of) hook(s). You say "all kinds of crazy things" but I say -- not at all. Image styles work. Should I convert fields as well to show you how well this works? Should I convert your vocabulary patch to this simple model? All that is doable and easily so.

catch’s picture

Why don't we allow individual APIs to handle their config objects in the way that makes most sense to them?

How's that going to work with #1447686: Allow importing and synchronizing configuration when files are updated, where you have a set of files and need to map it back to specific implementations of configuration objects? Or module install/uninstall?

I could see something like the sync operation not actually trying to save anything, and deferring all of that to hook_config_sync() implementations, but any solution here has to account for all the various starting points from which configuration can be saved.

sun’s picture

adding a (family of) hook(s)

...which run in no particular order, unless you require each and every API to also implement hook_module_implements_alter(), so it is able to ensure that every subsequent "API" consumer gets a config object that is prepared for whichever $operation.

We should postpone this on #1605324: Configuration system cleanup and rearchitecture


@merlinofchaos expressed agreement with #1, but raised the following concern against #3:

[regarding] Instantiating an object: A database system should not have detailed knowledge of the PHP objects in use. And Config is a database system.

This makes "some" sense, but I'm not sure how to "re-map" things yet. Also, to transport further discussion on this topic from IRC:

  • What we'll have to do either way is to not instantiate new DrupalConfig (the high-level storage manager/arbitrator) and storage controller classes for each and every config object that is requested. But at the same time, we still want to return a (PHP) object to consumers, so they can act on it.
  • Rough pseudocode:
    $data = config()->get('some key');
    $object = SomeSystemFactory($data);
    // later
    $data = $object->extractForStorage();
    config()->set('some key', $data);
    

    ...which basically argues for the "wrapper" object approach being attempted in the vocabularies patch?

    i.e., your module API gets a config object/data, and doesn't care at all what it is, instead turns it into a specific object that it creates on its own.

  • The general train of thought of existing feedback against that was that it allows to "hi-jack" and circumvent a particular module's API by doing "whatever" to the config data itself and saving it. In turn, no module (API) hooks are fired, and there's a risk of systematic inconsistency. Hence the idea of embedding the class name to instantiate, instead of a default ConfigObject, so that the special class implementation always executes what it needs or wants to. (?)
  • OTOH, I once argued against that by saying

    if you load + save such a config object directly, then you are intentionally omitting the module's API, which is identical to executing a variable_set() or db_merge() in D7.

    Long story short: You can update the {image_styles} table directly. But is it correct to do so? Certainly not.

  • But then, a reasonable argument against that was: How does a (innocent) developer figure out, which config can be operated on directly vs. which config has to be passed through special/dedicated module APIs?
  • The hooks we're talking about have to exist at the wrapper level, inside a controller's save/load mechanism, much like it does with entity.
  • But is it fundamentally wrong to dual-purpose the config object itself to be the object itself as well as the crud controller that handles load/save/delete in the case of config?
  • There might be use-cases where we want exportable things but which do not keep the actual in-use data in the active store, but instead will sync it with a database table. For that use case it would be nice to be able to not rely so heavily on Config.
  • Let's pretend there was a hypothetical CTools in D8 (let's hope not ;)) - can you see a problem with a base CtoolsConfigObject that implements all of that fanciness, which is the config object + special crud controller at the same time, and which all other modules, that depend on D8 CTools and which want to leverage its fanciness, need to use as base?

    The hope is obviously not, but we need to assume that the database-happy object *would* end up in CTools because getting it into core without a core-specific use case seems hard.

  • However - the previous point doesn't actually affect the "crud controller" part at all — it wants to override the storage controller!

    "CRUD controller" means current procedural API functions like image_style_save() or node_type_save(), which perform all kinds crazy shit. The above hints at a replacement for the storage controller of the active store (DatabaseStorage) - but - specific to a config object (e.g., a view). So you're able to "redirect" the regular config save operation into a different storage, outside of the configuration system.

    (That said, this would inherently detach all of that config from all the configuration system features we'll have in D8, which probably no one wants to do, but that's a completely different, separate, and fundamental topic we'll have to hash out ;))

So... I'm entirely not sure whether there is any conclusion to this (and if so, what it was :P ;)), but in general, I see many parallels and a desire to instantiating a config object-specific class. Unlike #1605324: Configuration system cleanup and rearchitecture has foreseen until now, this even goes one step further and demands for an ability to allow the config object to "replace" (or enforce) a storage controller with an implementation that is entirely different from the regular one(s).

All of that being said, I still have the impression that the views/ctools-to-config integration efforts are trying to bend the new configuration system to a level that architecturally do not make sense. At least within the context of the current config system. However, at the same time, I perfectly see and admit that the config system does not provide all options of the "exportables" system of ctools in D7. At the same time, I believe that we architectural design goals of the config system foresee that we do not front-load that import/export into every user's face and reduce it to plain import/export/sync operations triggered by site builders. But to complete this (heavily distorted) picture, it's perfectly possible that we're missing/ignoring some needed module/API config layer operations, which the current config system does not account for, and so at least a hypothetical D8 CTools should be able to inject them as necessary.

chx’s picture

...which run in no particular order

Hrm? We first run hook_config_presave, then hook_config_presave__image and so on. This is a well-defined order. It's the same as with hook_form_alter. As there is no $operation and most of the time the config object is not necessary anyways (although I pass it in) I can't make heads or tails of the second part of your sentence.

The rest is simple irrelevant here. Many times we have committed code we knew will be replaced with something better. Should something better actually happen. Please give me a use case from core or contrib that a) we want to convert to CMI b) we can't. My uses cases are a) image styles b) fields. field_sql_storage_config_presave__field($data)... and field conversion can happen. So what can't happen.

sun’s picture

FYI: We're currently detaching config objects from DrupalConfig, including storage controllers in #1605324: Configuration system cleanup and rearchitecture.

These changes are precursor for allowing us to do what has been outlined in #1 here, and thus, if all goes well, will inherently resolve the problem space of needing to know how configuration must be loaded, saved, or deleted, when all you have is the configuration data.

In turn, that would transparently make $config->save(); or $config->delete(); do exactly what it needs to do in each and every situation, and it's no longer anyone's hard responsibility to (somehow) figure out how some configuration should or must be saved.

gdd’s picture

I talked with chx and merlin and sun and msonnabaum about this some in IRC today, and one thing we discussed is whether or not we should be responding to config->save() at all. Merlin points out that ultimately the config system is not much more than a controller around storage, and that calling config->save() is not much different than calling db_update->execute() and nobody has ever suggested we respond to things at that level (with the exception of something like db_rewrite_sql() and we all know how well that worked.)

Lets look back at the scenarios sun outlines in the summary.

1) Save through an api call (image_style_save())
2) Installation of default config
3) Sync of config on deployment of new files
4) Reset to defaults

I've removed calling config->save() directly from this list, because anyone who does that can dig their own grave. That is consistent with our implementation of pretty much all other core subsystems.

Chx pointed out, and I believe he is correct, that 2, 3 and 4 are all ultimately implementations of the sync functionality. So really what we have is two operations:

1) Save through an API call
2) Sync of config after files are changed

What I don't understand in the code sample that sun pastes in summary.2 is why the sync code is calling module_invoke_all('image_style_save', $style); at all. My feeling is that any module's implementation of the sync hook should be calling its own API functions, and the hooks get fired from there. The proper firing of module-specific hooks should be the module's problem, not the config system's problem. If the module wants to duplicate code between hook_config_sync() and my_module_save() then the module is being stupid. The problem isn't in the config system, its that the sync code above is being dumb and/or code in image module isn't structured correctly.

So ultimately what I propose here is that this entire issue is moot. When you need to save something to the config system, use the module's API functions for saving, and everything is fine. This may possibly mean a bit of duplication in hook_config_sync() implementations, but I'm not sure I really care to be honest, given that it means the config system remains infinitely simpler. Sun started to go down this route in #33:

Long story short: You can update the {image_styles} table directly. But is it correct to do so? Certainly not.

But then, a reasonable argument against that was: How does a (innocent) developer figure out, which config can be operated on directly vs. which config has to be passed through special/dedicated module APIs?

I guess I don't think that 100% consistent DX across the entire system is an appropriate tradeoff for wrapping functionality into an architecture level where it doesn't belong. Entities work better for this, because the entity system only stores entities. We are storing 100 different types of things, we shouldn't try and force them all into one API because its impossible. We have this same problem today, some stuff is in variables and some stuff is in module-specific APIs. All we're doing is changing where the module-specific stuff is stored. Just because we're putting everything in the same place doesn't mean that has to surface all the way up the chain

chx’s picture

So to summarize: you recommend only firing hooks on config sync despite the operations needed there are the exact same as on save and the reason for that is save too low level? Sorry but this doesn't make any sense. Same operations, same code path. IMO if you make a new image style or a new field enter the system it doesn't matter how that happened, the same hooks should fire. You say that if possible (ie on API call) the hooks should be fired by the caller instead of CMI despite CMI needing to fire them anyways on sync?

But if this is what you want, I won't argue too hard, just please won't fix this issue and let's move over to the sync issue.

Anonymous’s picture

after further discussion in IRC, i'm coming around to the position that we shouldn't have a hook fire on $config->save().

merlinofchaos’s picture

chx: I disagree that the operations are the exact same.

If my $view object is firing config()->save() it has almost certainly already performed those operations. However, if I have to delay and perform those operations during the config save routine, I may have to re-instantiate my view object. That actually separates code and adds more code paths; it does not reduce duplication.

catch’s picture

Not sure I get this bit:

My feeling is that any module's implementation of the sync hook should be calling its own API functions, and the hooks get fired from there.

In #1447686: Allow importing and synchronizing configuration when files are updated the sync operation is writing to configuration, and image_style_save() is also writing to configuration, we don't want to do that twice. If we remove the actual configuration write entirely from the sync operation and devolve everything to modules, then that takes us back to a single code path though, is that what you mean?

sun’s picture

#1447686-129: Allow importing and synchronizing configuration when files are updated now contains a patch that turns hook_config_sync() into "change dispatcher" instead.

  • hook_config_sync() remains to be optional.
  • It allows a module to handle a particular configuration change on its own, using its own API functions, and possibly also higher-level configuration data object wrappers (e.g., $view = new View($new_config);).
  • If the hook implementation returns TRUE, then the config sync operation assumes that the module handled the configuration change and removes it from its stack of changes.
  • Any remaining changes that were not handled by a module are synchronized by the config system itself via direct read/write operations through the involved storages.
chx’s picture

What #39 says makes sense and I agree. It must be noted here that I did wish for an alter on db_update/db_insert several times but then again it's just me and I made my agreement already in general. And even that clause needs to be called on very rarely -- usually I just write a DB driver when the need comes, and so I expect I will write CMI engines. That's fine. We want the active store to be pluggable anyways.

fago’s picture

The proper firing of module-specific hooks should be the module's problem, not the config system's problem. If the module wants to duplicate code between hook_config_sync() and my_module_save() then the module is being stupid.

I fail to see how one would do so. If the API already gives me different looking variables to work with in both situations, I have to figure out the same code for invoking hooks twice. It puts the burden on the developers - consequently, I think that lots of modules would end up with not properly written hook_config_sync() implementations, making deployments troublesome further on.

However, #41 sounds like a reasonable approach to me.

gdd’s picture

Assuming that we are all in agreement on the solution in #41, it appears that resolution of this discussion has essentially fallen to the import/export mechanism, is that correct? If so, can we close this?

sun’s picture

Category: bug » task
Priority: Critical » Normal
Status: Needs review » Fixed

Yes. The approach outlined in #41 has additionally been discussed to great lengths in Barcelona and makes most sense for everyone.

The change was already incorporated into #1447686: Allow importing and synchronizing configuration when files are updated and #1626584: Combine configuration system changes to verify they are compatible, which thus both show how the basic implementation will work.

That said, the current patches are still missing the additional $dirty or $isNew flag/property/method on the Config object, which is going to replace the Boolean return value of hook_config_import().

Furthermore, the current code and tests for the import mechanism is still very basic in light of the overall Drupal module system complexity, potential dependencies, and other special cases. However, all work is currently blocked on #1605324: Configuration system cleanup and rearchitecture, and will only continue after that has landed.

catch’s picture

Category: task » bug
Priority: Normal » Critical
Status: Fixed » Active

Sorry how is this fixed? Are image style hooks fired when the default config is imported on module install? If not this is still a critical regression and needs to be tracked separately from any other refactoring. We can mark it duplicate of another issue, but then that also needs to be promoted to critical bug and have explicit tests if so.

sun’s picture

Title: Config saving is completely inconsistent in terms of hook invocation and etc. » hook_image_style_*() is not invoked for image styles upon Image module installation
Status: Active » Postponed

Ah. Sorry, I wasn't aware that you want to see a concrete fix for image styles in here.

In that case, let me relabel this issue to clarify the actual objective. I agree that this is a critical bug.

The installation of default config upon module installation is comparable to the import mechanism. Not exactly the same, but it should definitely re-use the same internal API functions and mechanism for involving the corresponding module API for a particular config object.

In Barcelona, @heyrocker, @merlinofchaos, @alexpott, @xjm, @webchick, and me discussed this mechanism and interaction. The solution proposal is the approach outlined in #41; i.e.:

  1. Config objects are plain/dumb config objects. Calling config($name) loads a plain config object, calling $config->save() saves a plain config object. The module/hook system is not involved at all. Period. :)
  2. A module may have a dedicated API for a particular type of configuration objects, such as image styles, views, node types, contact categories, etc. This module API is completely detached from the configuration system. Calling image_style_load($name) yields a/whatever "thingie" that Image module uses for its image style API. Calling $style->save() (or whatever) saves the thingie through Image module's image style API functions. Image module's image style API invokes module hooks.
  3. To repeat and clarify: By design:
    $config->save() != $style->save()
    
  4. Configuration object names are namespaced by owner (module). This means that foo.bar.baz belongs to Foo module.
  5. The import mechanism leverages configuration object namespace to determine the owner of a configuration object. Given the owner, it attempts to invoke a callback (not hook) in the owning module; i.e., MODULE_config_import(). If the module implements this callback, then it is asked to process the configuration data on its own:
    function hook_config_import($op, $name, $new_config, $old_config) {
    
  6. Given the configuration data, this callback basically acts as a dispatcher. It converts the raw configuration data into the appropriate "thingie" of its API and performs whichever actions need to be performed to handle the requested operation; e.g., in the most simple case:
      if ($op == 'create') {
        $thingie = new Thingie($new_config);
        $thingie->save();
      }
    
  7. Since there are many of these module API configuration thingies throughout core and contrib, it is our intention to design a common ThingieInterface and perhaps even a ThingieBase that can be extended, with the goal of providing a comparable DX as with entities. Please note, however, that the entire Thingie stuff is technically out of scope for the configuration system. The configuration system's involvement and responsibility ends with the callback invocation. Nevertheless, the same people are going to work on it, of course. ;)

Postponing on #1605324: Configuration system cleanup and rearchitecture (RTBC). As soon as that lands, my plan would then be to complete the combined patch in #1626584: Combine configuration system changes to verify they are compatible with a thingie for image styles (ImageStyle class) to figure out and double-check how the import mechanism functions can be re-used for the module/extension installation, and come back here with the extracted changes afterwards.

sun’s picture

Assigned: Unassigned » sun
Status: Postponed » Needs review
FileSize
29.56 KB

Actually, working further on this, I figured out that converting image styles from arrays into "classed thingies" is totally not required for fixing this issue. Not having any presumption or dependency on the module API level is actually exactly one of the primary design goals of the module callback approach.

Obviously, this patch contains a range of changes from #1447686: Allow importing and synchronizing configuration when files are updated - but this is the common denominator of functionality only. The main config_import() and config_export() functionality for bulk-synchronizing files with the active store is still left for the original issue.

I think it makes sense to perform this change separate from the actual import/export feature, since this issue is actually about a critical bug/regression of not invoking module hooks anymore, and the main discussion on how to resolve that problem is also in here.

There do not seem to be any tests for Image module that verify invocation of module hooks. I made sure to additionally check the original code before the initial #1270608: File-based configuration API landed.

Either way, image style API hook invocations are only the symptom of this issue. The actual issue is that any module API is not invoked. Therefore, this patch introduces the config_test module, which implements the most basic functionality for handling dynamic configuration objects. Of course, it is invoking API hooks for its own ConfigTest thingies upon loading/saving/deleting. And that's the essence of this issue :)

Thus, instead of adding one-off tests for Image module, we're adding generic tests for a generic testing module. As such, config_test is fairly similar to entity_test module, which similarly implements a basic but fully operational generic entity type.

Note: This branch/patch originally contained #1666632: Add Config::isNew() to allow code to determine whether a config object already exists, and reverting those changes wasn't exactly trivial, but hopefully, that will land in the next hours.

sun’s picture

That said, if you guys prefer to skip tests entirely for this issue/patch, then here's the stripped down patch. ;)

#48 as well as #1626584: Combine configuration system changes to verify they are compatible actively confirm that this is working, since the tests will be introduced at some point.

One potential reason for going without the tests is that the config_test module introduces a "configurable thingie" (ConfigTest class), which I still need to discuss with @merlinofchaos and others. And although testing module code would not imply any architectural assumptions or best practices, I could very well understand a potential pushback against the addition of that code.

I'm happy to go either way. Would almost prefer to go without tests here. But I don't really care.

EDIT: What I actually care for (in case it's not clear) is to resolve this critical bug to get back below critical/major thresholds.

gdd’s picture

Status: Needs review » Reviewed & tested by the community

I think I would prefer to go without the tests too, so that would mean #49 is the one to commit. I don't want to include the configurable thingie concept until we've had time to discuss that in its own place.

I've looked this over and its a really nice patch. I'm not a huge fan of the NullStorage solution which seems like a bit of a hack, but its a graceful hack and I wouldn't be surprised if we find other uses for it down the line.

Thanks sun.

sun’s picture

Thanks! :)

Also, to clarify: The only test failure of #48 was: Fatal error: Call to undefined method Drupal\Core\Config\Config::isNew() ...so apparently, my partial revert of the isNew() changes didn't go so well ;) As mentioned, we will pretty much notice immediately if this would break due to the other issues/branches.

I'd prefer to go with #49 as well.

Second, there has been some more discussion on potentially leveraging a property/flag on Config objects for the module callback dispatching logic recently. However, that is definitely not essential for this issue and can/could be easily changed later on. The main comments on that happened in #1447686-132: Allow importing and synchronizing configuration when files are updated (and following), so it only makes sense to continue there and ignore it for the critical bug that is fixed here for now.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Goodness, that's a lot of code to be adding without any test coverage. :( :( However, I understand the sequence that's desired here, and discussing "Configurable thingie" as a concept separately makes sense. I think we need a critical task as a follow-up to make sure those make it in and we don't forget about them. On IRC sun and I had discussed making this issue #1668820: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc), or possibly #1447686: Allow importing and synchronizing configuration when files are updated once that question had been settled, but the synchronization issue is flamingly critical and it doesn't really make sense to block it on what's essentially a DX feature.

So. Critical follow-up for test coverage, postponed on some combination of those issues, but with the fact that we want #1447686: Allow importing and synchronizing configuration when files are updated to be done as soon as possible so we can actually (manually) test the configuration system. I'd even be fine with introducing ConfigurableThingies within the tests only and then using that as a discussion point for the question of broadening that to the whole API.

Other things:

+++ b/core/includes/config.incundefined
@@ -57,3 +68,111 @@ function config($name) {
+ * Writes an array of config file changes from a source storage to a target storage.

80 chars. Can probably omit "an", "a", etc. and get it to fit.

+++ b/core/lib/Drupal/Core/Config/NullStorage.phpundefined
@@ -0,0 +1,61 @@
+ * Defines the Null storage controller.

..and? :) What does it do, what's it for, etc? If I'm a developer new to the configuration system and scanning through the list of storage backends, what makes this one special?

+++ b/core/modules/config/config.api.phpundefined
@@ -0,0 +1,63 @@
+ * (such as image styles, node types, or fields), which needs to be

No comma.

+++ b/core/modules/config/config.api.phpundefined
@@ -0,0 +1,63 @@
+ * @param Drupal\Core\Config\Config $new_config
+ *   A configuration object containing the new configuration data.
+ * @param Drupal\Core\Config\Config $old_config
+ *   A configuration object containing the old configuration data.

Elsewhere the API does "old, new" so I think we should swap the order of these parameters. Maybe?

+++ b/core/modules/config/config.api.phpundefined
@@ -0,0 +1,63 @@
+  // Only configurable thingies require custom handling. Any other module
+  // settings can be synchronized directly.
+  if (strpos($name, 'config_test.dynamic.') !== 0) {
+    return FALSE;
+  }

Should we make two hooks then? Copy/pasting this check in every hook implementation seems ugh.

+++ b/core/modules/config/config.api.phpundefined
@@ -0,0 +1,63 @@
+  if ($op == 'delete') {

In fact, shouldn't we really make these separate hooks entirely? hook_config_import_delete(), hook_config_import_change()... we tried very hard to obliterate $op everywhere in D7.

+++ b/core/modules/config/config.api.phpundefined
@@ -0,0 +1,63 @@
+    // @todo image_style_delete() supports the notion of a "replacement style"

We can probably remove this @todo code from the sample code. Else change it to @todo: update based on changes in image_config_import().

sun’s picture

1) On the NullStorage docs, it does the same as the NullBackend of the cache system - nothing. We can copy/paste most of the docs from there. I think that could be done in a follow-up (e.g., #1516472: Missing/incorrect API documentation for configuration system), but ok...

2) On the new, old callback argument order, I guess you're talking about the $source_storage and $target_storage parameters in config.inc. However, those have an entirely different purpose, and they are not part of the public/module-facing API. For the module callback, I think it makes more sense to pass the new config first, because that is the main parameter that modules will interact with.

3) On the hook_config_import() splitting, the config system does not know whether it deals with a configurable thingie. That's what the callback is for.

4) On splitting hook_config_import() to get rid of $op, there's always something you have to copy/paste. With separate hooks, you'd have to copy the initial condition into every _create/_change/_delete callback, and the callbacks would need to create helper functions for sharing any possibly required additional code between $ops. Given that, it seems more logical to me to route by $op in a single callback.

5) On the @todo, no I don't want to remove essential @todos just yet.

webchick’s picture

Discussing this more in IRC there was general agreement around:

1) Get rid of $op; this is the standard hook pattern even if it does ultimately mean more copy/paste of those 3 lines of code.

2) We need docs along the lines of http://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Cache%21Nu... for the NullStorage class; just a paragraph or so, that's about it.

The rest we can punt on.

sun’s picture

Status: Needs work » Needs review
FileSize
27.5 KB
18.35 KB

Fixed both issues.

Also, on the grounds + hope that this will land today, I've disrupted the extraction process and removed all configurable thingie parts from the patch so that we can commit the test with this patch.

4aca5c4 Removed $op from MODULE_config_import() callback.
24b3b35 Added docs for NullStorage.
65fe3b6 Merge branch 'config-next-1626584-sun' into config-callback-1609760-sun
23eb57f Removed irrelevant thingie changes.

webchick’s picture

Status: Needs review » Fixed

Ok, looks like that addresses my feedback; thanks!

Committed and pushed to 8.x! Thanks a lot.

chx’s picture

Title: hook_image_style_*() is not invoked for image styles upon Image module installation » Roll back hook_image_style_*() is not invoked for image styles upon Image module installation
Status: Fixed » Active

I no longer can keep track of which issue is which CMI-wise so it is possible this was not posted into this issue but I know it was posted somewhere and I want this rolled back. I have argued hard and long and I thought I agreed with sun that we will always do a save. Here's the relevant IRC log from not a week ago

[Friday, June 29, 2012] [04:34:23 PM] <sun> you're ok with the dispatching to a module API. All you want is that none of the dispatched config objects is ever removed from the stack of config changes. Instead, the
entire original stack should be passed to config_sync_changes(), and for each object, the $isNew or whatever property is checked.
[Friday, June 29, 2012] [04:34:43 PM] <chx> Instead, the entire original stack should be passed to config_sync_changes(), <= YES YES
[Friday, June 29, 2012] [04:35:07 PM] <chx> and for each object, the $isNew or whatever property is checked. <= helps performance, yes but it's not fundamental

chx’s picture

chx’s picture

And the only reason this could slipped through me is because it was never marked RTBC. That's reason enough to roll it back: I am following the RTBC queue even on my phone to make very sure I see every patch that goes into D8 and then this happens. Edit: OK this was RTBC three days ago, my bad for missing it, then.

sun’s picture

  1. Yes, there has been a proposal to use a flag/property/whatnot on the Config object to track whether it has been handled by the module callback. This was pointed out in #51.
  2. http://drupal.org/node/1447686#comment-6182056 contains a list of problems with that proposal, which could not be addressed by those who proposed it.
  3. The suggested flag/property change is an unimportant detail for this critical bug of not invoking hooks for installed configuration anymore.

In turn, I don't see why this should be rolled back, and why — if at all — it can't be changed in the import issue or separate follow-up issue.

chx’s picture

It was not the propery that is important I told you crystal clear until you got it, see the quote in #57. It is that we save every CMI object. You misrepresent of what I was talking about despite I also said, repeatedly "and for each object, the $isNew or whatever property is checked. <= helps performance, yes but it's not fundamental" (and the property is not $isNew but tainted, but who cares)

And it can't be done in a followup because then that might or might not be committed and that's simply wrong. I really do not know how to better do this, I thought we had a discussion ongoing in the sync issue and then suddenly the relevant parts of the sync issue appear in the core! No way.

chx’s picture

Title: Roll back hook_image_style_*() is not invoked for image styles upon Image module installation » CMI is no longer centralized
Status: Active » Needs review
FileSize
3.79 KB

Seems I make more sense when I talk in PHP.

sun’s picture

The effective result of what is being saved after being passed through a module API can depend on other factors in the system/environment.

Therefore, I believe it is wrong to overwrite what the module API saved. Doing so will only hide errors and effective differences.

chx’s picture

The module API should not save anything. It should fire its hooks, write its own storage if it deems that necessary and that's it. We do not say that "hey, this was handled in hook_node_presave, let's not save it!" that's wrong API wise. It's the task of the system to save not the task of a module.

chx’s picture

Note however how claimed misunderstandings which I do not beleve for a second have successfully moved the issue from let's roll this back to let's fix it.

gdd’s picture

hook_node_presave() is a fundamentally different architecture. For nodes we say 'Hey Drupal is about to save a node, is there anything you want to do before we save it?' and then after modules do their thing, the node system has more work to do for all nodes, like setting updated and the like. For config we say 'Hey there is some new config, here it is do with it what you will.' It's not like there is any functionality the config system needs to do in addition to the saving, and to force module developers to set a flag saying 'Hey config system, you should save' seems like we're making the DX more complex with no real benefit. I know that in the past chx has had issue with this, because it will allow module developers to essentially bypass CMI and write their own system for storing configuration. However, I think this concern is overblown. Nobody would use such a module (just like people dont use modules now that dont support Features) and if you want to do it for your own custom site, well, dig your own grave. There is no usecase for bypassing CMI, it doesn't make anybody's life easier, and I don't really think that this is something worth worrying about.

chx’s picture

If there is no usecase for bypassing CMI then why my vastly simplified patch is not considered?

chx’s picture

Note: I unfollowed the issue. I said all I had to say, the patch is filed, it simplifies and passes tests. There's no more I can or want to do.

sun’s picture

Title: CMI is no longer centralized » hook_image_style_*() is not invoked for image styles upon Image module installation
Status: Needs review » Fixed

In that case, we can properly discuss the suggested tweak to the import mechanism in
#1670370: Enforce writing of config changes during import, even if the module callback handled it already

As already mentioned way back in #51, that's only a very small detail of this change, so I don't really see why we need to revert the entire commit.

Note: I still don't understand what the problem is with leaving the saving to the module callback, so I'm not able to write a proper issue summary for the dedicated issue. I'd highly appreciate it if someone could fill that in.

Anonymous’s picture

Title: hook_image_style_*() is not invoked for image styles upon Image module installation » CMI is no longer centralized
Status: Fixed » Needs review

re #63 - i'm a bit concerned by that statement.

i think it's saying "modules can alter config data, so what is saved to the active store may be different to the what is in the source config object". am i getting that right? and that is a use case that we want to support? a feature, and not a bug?

i thought we called out to module code during a sync operation for two reasons:

1. so that modules can react and change things *outside the scope of CMI storage*. like, create tables for a field, do something in views-land, whatever. anything beyond just saving the declarative changes in the new config files.

2. allow modules to write to the active store during a config sync operation, so that dependency issues can be resolved. for example, given a new entity and a new field on that entity, we may need to write to the active store during the config sync.

so can someone elaborate a bit on why we want to support 'the config changes you push may not be written as you pushed them' as a feature?

xjm’s picture

When we disagree, we consult others.

@sun and @chx disagree about the specific code seen in chx's patch in #62. In this case, I'd interpret the DCOC as indicating that sun should not set the issue back to fixed, nor should chx insist that it be rolled back. Instead, wait for others to review #62.

I'd also like clarification on #70 in order to consider #62. Edit: A specific example would help me.

chx’s picture

Be collaborative.

I can quote the DCOC too!

xjm’s picture

Yep, I think that also applies. :)

chx’s picture

Note: sun tried to derail this into another issue. No way we will have another issue there are more than enough already that's how this got in the first place. Also, if the module decides to change $new_config (which i do not think it should) then we are still good because I do not save $data, I save $new_config->get()...

sun’s picture

I've provided examples and a test to assert my expectations in #1670370: Enforce writing of config changes during import, even if the module callback handled it already

Also, to clarify once more:

The only reason this issue continued after #45 and did not melt back into the original import issue was @catch's very valid point in #46:

Sorry how is this fixed? Are image style hooks fired when the default config is imported on module install? If not this is still a critical regression and needs to be tracked separately from any other refactoring.

So after collectively concluding and agreeing that we need to introduce a dispatcher mechanism in the form of a module callback, in order to allow the module that owns a configuration object to take over its saving/deletion through its regular module API, and after figuring that the installation of a module's default config should work in the same fashion, it appeared sensible to extract only the minimum required changes from the import patch, as to be able to fix this critical bug/regression.

Thus, with current HEAD, the above question can be positively answered with:

Yes, Image module's image style API hooks are invoked for its default config upon installation, because Image module saves the image style!

The question of whether Image module should have any chance to adjust the config object that it owns, or whether it shouldn't, appeared to be peripheral to the main challenge of getting the module API involved in the first place. And apparently, that exact question was not even considered in the discussion, since there was a larger misunderstanding in the actual change that was suggested. Instead, we discussed the question of whether there should be an internal property on Config objects, which attempts to track whether the object has been passed through a storage controller, and a couple of concerns were raised against that idea (in the original import issue, on which the change was also proposed).

Overall, I'm very happy that we've been able to resolve this critical issue. I believe that it's perfectly valid to move the discussion about the detail of what exactly gets written into #1670370: Enforce writing of config changes during import, even if the module callback handled it already (or alternatively, back into the import issue, although that issue apparently is overloaded with a lot of details already), and FWIW, I also think it's fairly foreseeable that this won't be the last follow-up issue for this functionality.

chx’s picture

Nope. We are not moving this discussion away. You have deceived me by channelling this discussion to be about the property when it was clearly not about it and you knew that, see IRC quote in #57 about it. Whether modules can change their config is another question I did not want to discuss here nor it is relevant. #62 is about always saving the new config. And that's the patch to review. I will not let you derail this conversation again nor take it to another issue so that changes can't be tracked any more.

sun’s picture

Someone abused user privileges to close comments on #1670370: Enforce writing of config changes during import, even if the module callback handled it already, so moving over to this issue.

Let me try once again: Is anyone able to fill out the missing "- Unknown -" gaps for the actual change proposal of that detail?


Problem

  • - Unknown -

Goal

  • Make the import mechanism save every Config object during an import, regardless of whether is has been handled by the owning module callback already.

Details

  • - Unknown -

Proposed solution

  1. Invoke the MODULE_config_import_*() callback for a Config object to allow the owning module to take over the saving/deleting.
  2. Overwrite the Config object saved by the module API with the exact data from the source storage (or respectively, enforce its deletion).

Counter arguments

  • Overwriting the config object after the module API saved it will potentially overwrite differences. The effective result of what is being saved after being passed through a module API can depend on other factors in the system/environment. Doing so will only hide errors and effective differences.
sun’s picture

Attached patch contains a dead simple test assertion that demonstrates what a module API might reasonably do.

This is just one of many possible examples. It's most likely the worst example of all possible. Much better and more reasonable examples would be:

- Ensuring default values for all properties.

- Injecting system/environment-specific values. (e.g., auto-generating the system.cron:key in case none is defined yet)

- Validating references to other resources in the system, and possibly adjusting a status/enabled flag depending on their existence in the target system.

- etc.


Overwriting the config based on $new_config does not help.

1) There is no guarantee that the module API saves the $new_config object. This breaks the assumption:

  $config = config($new_config->get());
  $config->save();

2) There is no guarantee that the module API uses the $new_config object. There is no requirement for the module API to be based on the literal Config object as is. A module may use a completely different way for managing its dynamic configuration thingies. Have a look at Image module, whose image styles happen to be plain arrays:

  $style = $new_config->get();
  return image_style_save($style);

This essentially repeats #132 and #136 from #1447686-132: Allow importing and synchronizing configuration when files are updated

chx’s picture

Problem: you have no idea what the site configuration is because it is scattered nilly-willy in whatever places a module fancies to store them. #66 says this won't happen. I say, wishful thinking.

I seriously do not know what else to say. What "details" do you want? And, the patch in #78 does not break #62 (which probably should use $new_config->save() but that's again immaterial) as I have said several times. Why would it?

webchick’s picture

Wat? :(

chx’s picture

wat? After #1605324-48: Configuration system cleanup and rearchitecture and spending the time to explain my concerns about the sync issue to sun, he have, again, brushed me aside and I blew a gasket over that.

sun’s picture

It does not make sense for me to continue this discussion, because the communication I'm experiencing is overly aggressive and keeps on personally attacking me. Even calling this a "discussion" is an extreme extenuation, the very first action was "fuck you, roll this back." — The entire hyperbole and drama is beyond me. I've ignored the personal attacks and FUD/BS, and tried very hard to extract and provide reasonable and technical arguments for the sake of making productive progress, but not even that helped in any way. People are not running short in quoting the DCOC, but what happened to assuming and considering best intentions? Yes, I guess it is easy to shit and piss all over the work of others and don't give a damn for how long and how much hard work it took to get to a result. And I guess personal attacks is just the minimum you get in return for contributing to Drupal. If it wasn't me, then seeing this kind of "discussion" would seriously make me think thrice before attempting to contribute anything. But alas, that's no news for me, so don't worry about me. Lastly, let me amend that the current status of this issue is a deadlock for pretty much all other config system issues, because I have no idea whether this is going to be rolled back or not, and I will certainly not waste my time with re-rolling/rebasing a dozen of other issues with hindsight of potentially having to redo everything differently.

chx’s picture

It will not be rolled back. #62 is a patch to review, it does not involve architecture. The personal parts I am not reacting to.

chx’s picture

And, the work I am grateful for. Really.

sun’s picture

#78 invalidates #62.

No technical reason has been provided for #62 so far. The only provided reason is cryptic (to say the least) and solely based on fear:

you have no idea what the site configuration is because it is scattered nilly-willy in whatever places a module fancies to store them.

It's close to impossible to work with and argue for or against such statements. The way I interpret it is that it basically tries to say:

"A module API cannot be trusted."

If that is the sole base of the argument, then it is invalid. Because this yields the same:

$vocabulary = taxonomy_vocabulary_load($vid);
$vocabulary->save();

If you don't want to use the module's API, then you better don't use the module.

sun’s picture

On top of #78, I just wondered whether there is any need for the "export API version" that happens to be the base integration part of CTools' Export API, Views API, Panels API, etc. within D8's config system.

The current system apparently assumes always-compatible config objects. And we know that this is an invalid assumption.

E.g., admin_views.module can provide default config (views) for views.module, but which are only compatible with views 3.x, and on top of that, they also require VBO 2.x. The views definition is only compatible with Views 3.x and incompatible with Views 2.x. The VBO plugin definition is only compatible with VBO 2.x and incompatible with VBO 1.x.

Thus, either all modules are forced to work with any kind of version-agnostic config at any time — which is the current situation and effectively means "backwards-compatibility for eternity."

Or we not only need to trust the module API, but we actually need to introduce an additional mechanism/way to allow modules to "reject" config if it is not compatible during any kind of installation/import.

The more I think about this, the more it invalidates the idea of always-identical config on import (#62).

This detail is such a huge topic on its own, I seriously cannot understand the logic behind mixing the discussion into this issue. However, I won't argue against this any further, since there's no point in moving the discussion again now.

Anonymous’s picture

had a long discussion with sun and chx about #70.

to try to summarise:

  • my concern in #70 was based on an assumption that during an import (not module enable) we would write the same data to the active store as what was in an exported config file
  • i'm alone in this assumption. i can live with that, so i'll withdraw my objections to the code that was committed in this issue
  • the disagreement between sun and chx on this issue is whether or not to trust that modules will write out config. i don't have a strong opinion either way, so i suggest if the two of them can't work it out, let's invoke the heyrocker hammer. doesn't feel like a big issue to me
chx’s picture

"If a control always need to be operated do not provide that control" Jef Raskin, The Humane Interface ( I hope I quoted it right). This is true for API as well as UI design.

sun’s picture

Paraphrased and anonymized third-person transcript from my IRC discussion with @beejeebus:

  • There has been an original assumption that when we import new config, we write out the values from the import tree exactly as they are.
  • But now that we are actively involving the module API, it appears wrong to some of us to overrule the module API (which owns the config object).
  • The provided examples about setting defaults and stuff do make a point. They are not necessarily correct, but they are cases we need to consider.
  • Taking this "feature" into consideration, one could (hypothetically) write a new view config file with just, say, it's "name" property, and assume that Views will fill in a bazillion default fields for me. And there's not really something wrong with that.
  • The original assumption was that such a config would be rejected by the import mechanism. Specifically with regard to scenarios, in which you push from dev/test to production. In that scenario, you export config and push them. And you do not expect to see differences.
  • Technically, that is already the case. The module API just saves what it gets/imports (i.e., idempotence in ->load()->save()). So you do not see a difference.
  • However, if we'd just simply overwrite what the module API wrote before, then we apparently would exactly not show you a difference.
  • If you'd want to see/catch any errors/differences during/after import, then we'd rather have to invoke the module API, and after doing so, reload what has been written, and compare that with the config that was supposed to be written originally.
  • So, on one side, we have this idea of "what goes in from an import is what goes out" vs "modules may augment imported config instead of rejecting it". The idea of setting defaults etc seems to be very useful. That would be a real feature, but it does complicate the assumptions around the system.
  • The idea of strictly saving what's supposed to be imported is well understood; many of us assumed that for a long time. But it does not necessarily make sense to enforce this rule at all times (specifically with regard to installing a module's default config).
  • It might be possible to add a flag to the import mechanism to conditionally enable that post-module-import-callback-validation for "true" imports from exported config files -- although it's not clear whether that would help.
  • You do not really care for whether the default config ends up 1:1 in the system after installation. You only care about that in the situation in which you import from exported files (or more generally, the passive store). In that case, we can make all sorts of assumptions about the target system.
  • So we could add another argument to config_import_invoke_owner(), which has a reasonable default, but which essentially enables that post-import-validation after invoking the module API by comparing the effective result with the expected result, and somehow, erroring out on a mismatch. That, in order to validate "what gets written is the same as what goes in."
  • Actually, the module callback might have to decide on its own whether the config to be imported shows essential differences, so as to make a clear difference between error conditions and minor diversions.
  • In any case, the module code has no idea about the system it is being installed on. It could be a fresh install, could d.o., whatever. So, when importing module config at install time, it makes sense that what gets written may differ, because many modules augment others, or add something to each view or whatever. So you can't look at the set of module default config files and say "ok, that is what is in the active store".
  • However, this is also the reality of having to deal with a multi-dimensional module scenario. Because you do not know what the target system/environment is.
  • In turn, the question pretty much boils down to: In a staging scenario, you can and should reasonably expect identical import results if your target system/environment is 100% identical to the previous dev/stage environment. And, there is no reason why the import would yield different results if everything is identical.
  • You can only get differences if the target environment is not the same. And this is where the conflict with original assumptions appears.
  • The original assumption is that this does not feel right. The system should do what it is supposed to do, the obvious thing. In natural language, "import this config", just that.
  • However, if you want to make the system to do exactly what you think it should do, then you just let it do its thing. If, for whatever reason, that turns out to not yield the result you were intending, then you better know by seeing differences, instead of failing + overwriting silently. That's a valuable point.
  • At the point you're writing the exact data that lives in files, you're eliminating the potential difference. So we should probably blow up upon detecting a difference. Although that begs the question of "how".
  • There are two options: 1) Just overwrite what the module API might have written. 2) Detect a diff and blow up.
  • But it really boils down to trusting the module API, or not. If we do not trust it, then we must always write, and we don't care if what is written doesn't match what was exported. If we trust it, then we trust what it writes.
  • The original assumption also contained: "Look, I exported that tree, that's what prod looks like." + We're building something to move config between environments. But now, we're building something that allows you to take some config from anywhere, try to import it.
  • If you want to see what prod looks like, just export it. That's always going to be 1:1 export. But if you want import that tree of config, then you need a system/environment that is 100% identical to the one it was exported. If that is the case, no differences.
  • Only if the target system/environment is NOT identical, you naturally get differences.
  • So essentially, an exported tree from a dev/staging environment is just a "guide" to what will be written to your prod system.
  • And yes, we're going to run into epic/monster bugs with the export/import mechanism, since we didn't even think a minute about module update paths and version conflicts. But luckily, that's a topic for code freeze.
  • It probably wouldn't be just a guide, if there was an overall system compatibility validation operation happening before the import -- e.g., comparing enabled modules, their versions, Drupal version, PHP version, PHP extensions, etc.pp. - whereas that overall system validation would be an attempt to validate 80-99% system equality before the import.
  • And that's pretty much the essence: If you make the assumption that the source and target systems are 100% identical, then no module API will yield any differences in what is being saved. It is probably safe to make that assumption.
  • But in light of that, an overall system validation as a first step before trying to import exported config might make sense, too. That overall system validation might help us to resolve conflict scenarios later -- e.g., if you push new code + new config at the same time, the system would be able to tell you that you first need to run update.php, before you can execute the import.
  • As a result, the only disagreement is whether to trust module APIs to write changes or not. It appears to be a minor detail, but apparently questions the entire concept.
chx’s picture

Title: CMI is no longer centralized » hook_image_style_*() is not invoked for image styles upon Image module installation
Status: Needs review » Fixed

OK I am done with CMI for the foreseeable future, do whatever you want. Really. I do not know what to say to this epic rant when I have said on IRC that the "what to write" is not what this issue but it is a followup. Rather, this issue is about "do we write or not". That was, again, ignored. Given that the whole issue blew up because sun ignored me there is nothing I can or want to do: he won by attrition. Apparently this is how it is and my opinion is no longer sought or valid. It was not easy to reconcile with how different the world view of WSCCI is compared to mine and it will be even harder to reconcile when here my opinion is deliberately swept aside, the few issues I participate are forked, derailed and drown out until I can't even be heard. Well, then I will withdraw from CMI discussions as I did from WSCCI.

Edit: because the above meticulously left out everything I said, including how the epic rant is a followup and not on topic, here's my part in that IRC convo where I have tried to keep the discussion on topic (and it actually went better than this implies, I thought we have an agreement):

[Wednesday, July 04, 2012] [06:39:12 PM] <chx> I do not know why we are spending days and dozens of followups on something this simple: whenever we bring in a new text file there needs to be a guarantee that we write the active store so it can be re-exported
[Wednesday, July 04, 2012] [06:41:40 PM] <chx> that is a followup whether we do $target_storage->write($data) or $target_storage->write($new_config->data()) . Both are workable from my end and which one gets in is none of my concern.
[Wednesday, July 04, 2012] [06:43:07 PM] <chx> How does this invalidate my argument there needs be a guarantee of writing the target store and currently there is none?
[Wednesday, July 04, 2012] [06:44:50 PM] <chx> => I do not know why we are spending days and dozens of followups on something this simple: whenever we bring in a new text file there needs to be a guarantee that we write the active store so it can be re-exported <=
[Wednesday, July 04, 2012] [06:53:52 PM] <chx> write what up?
[Wednesday, July 04, 2012] [06:53:56 PM] <chx> Another derail of the issue?
[Wednesday, July 04, 2012] [06:53:58 PM] <chx> No way
[Wednesday, July 04, 2012] [06:54:05 PM] <chx> Absolutely no way
[Wednesday, July 04, 2012] [06:54:15 PM] <chx> The issue is about writing the active store always and not what we write in there
[Wednesday, July 04, 2012] [06:55:44 PM] <chx> beejeebus: you stalwartly concerntrate on what to write
[Wednesday, July 04, 2012] [06:55:47 PM] <chx> beejeebus: that's not he question
[Wednesday, July 04, 2012] [06:55:52 PM] <chx> beejeebus: the question is whether we write or not
[Wednesday, July 04, 2012] [06:56:04 PM] <chx> beejeebus: i asked, i can check the log, whether i asked thrice or only twice
[Wednesday, July 04, 2012] [06:56:12 PM] <chx> => I do not know why we are spending days and dozens of followups on something this simple: whenever we bring in a new text file there needs to be a guarantee that we write the active store so it can be re-exported <=
[Wednesday, July 04, 2012] [06:56:17 PM] <chx> beejeebus: where is the answer to that?
[Wednesday, July 04, 2012] [06:56:22 PM] <chx> beejeebus: that's the issue about
[Wednesday, July 04, 2012] [06:56:26 PM] <chx> beejeebus: that's what the patch does
[Wednesday, July 04, 2012] [06:56:40 PM] <chx> beejeebus: all i hear is the discussion about what to write not whether to write.
[Wednesday, July 04, 2012] [07:17:54 PM] <chx> back to this one, anyone care to 1) tell me to move module_invoke down 2) RTBC the patch 3) tell me what 3) is.
[Wednesday, July 04, 2012] [07:41:21 PM] <chx> it's imo a crappy api that relies on the consumers to do the right thing
[Wednesday, July 04, 2012] [07:41:27 PM] <chx> that's not a robust api

sun’s picture

Status: Fixed » Needs review

After all the follow-up discussion that happened, I do not accept an emotional outburst as a valid reason to mark this issue fixed.

Before we can mark this fixed and move on, there has to be an agreement on the remaining question/disagreement of whether to trust module APIs for writing import config data or not.

Reverting the title makes sense though, so leaving it at that.

sun’s picture

Essentially, if we trust module APIs to do the right things, then the patch in #78 should be marked RTBC - it testifies the expectation.

sun’s picture

This issue still blocks other config system issues. We need to decide on one of the two options:

1) Commit the follow-up patch in #78, which essentially means that the configuration system trusts module APIs to handle the to be imported config objects correctly, and also write/delete them correctly. It may be desired to add an optional post-import-validation step to this, so certain import scenarios are able to verify that the imported configuration is identical to the exported configuration, but that would be left to a separate follow-up.

2) Commit something along the lines of #62, which essentially does not trust module APIs at all, and only invokes them so they are able to invoke module hooks and whatever else they may want to do. The config system overwrites what the module API might/would have written.

The summary of the remaining issue is in #77 and below.

If you're not familiar with this entire issue yet, I'd recommend to either read the entire issue, or from #46 and below.

@heyrocker stated his opinion in #66 already, but it is not clear whether that was a "heyrocker hammer" already.

chx’s picture

It's impossible to resist commenting on such a false either-or. #62 does not contradict #78. I said this already, in the issue, in IRC, in code and in English, but let me repeat in code: we save $target_storage->write($name, $new_config->get()); and not $target_storage->write($name, $data);. If the module has chosen to write, that's fine, we are just repeating the same save. We might want to run $new_config->save(); instead but that should be the exact same as #62 already and as a followup investigage whether we can avoid a double save.

The question "do we trust the module APIs?" is a false dichotomy again: it's not about trust, it's about creating a simple-to-use and robust API. #62 simplifies the existing code and does not break anything AFAIK -- not #78 as far as I can see.

chx’s picture

FileSize
5.59 KB

Here's #62 and #78 rolled into one to prove they dont contradict. Drupal\config\Tests\ConfigInstallTest passes locally for me. To prove it simplifies the code: 4 files changed, 27 insertions(+), 46 deletions(-) and 9 insertions are in the test code.

sun’s picture

To prove that #62 contradicts #78.

Status: Needs review » Needs work

The last submitted patch, drupal8.config-import-module-api-test.96.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
870 bytes

You mean, here's a new test that #62 fails. I argued so far that #62 does not contradict #78 and it didn't so you have come up with a completely new test -- whether we care or not about that one is a question I leave to others. Is it hilarious or sad that you again failed to get facts straight? So, #95 was #62 and #78 rolled into one and here's the difference between #95 and #96.

gdd’s picture

I feel like there is still discussion to be had on this issue, and I don't really like wielding the hammer much anyways. However this is definitely blocking import/export which is a HUGE thing we've been trying to get in almost since freaking BADCamp of last year. So can we at least agree to move the disagreement to a followup, with a fresh summary, and mark this fixed? Chx I know you won't like this and I sympathize, but I do think it is necessary for us to move forward with other patches.

gdd’s picture

To clarify, I suggest we

1) Close this issue and mark it fixed
2) Reopen #1670370: Enforce writing of config changes during import, even if the module callback handled it already (where I have turned comments back on since they never should have been turned off)
3) Proceed with #1447686: Allow importing and synchronizing configuration when files are updated using the current core code, and when #1670370: Enforce writing of config changes during import, even if the module callback handled it already is resolved, bring in the results (if #1447686: Allow importing and synchronizing configuration when files are updated hasn't been committed already.)

This has several advantages. It unblocks #1447686: Allow importing and synchronizing configuration when files are updated so we can get back to work on it. It also removes the item of dispute from this issue (which is crowded with unrelated discussion) into a focused issue where it is more easily followed and discussed.

The code has been committed and apparently won't be rolled back, so this seems to be the best course of action. For the greater good of the system and moving forward, I hope this can be agreed on.

webchick’s picture

Status: Needs review » Fixed

I support this. Only change is let's escalate #1670370: Enforce writing of config changes during import, even if the module callback handled it already to major so we don't lose track of it.

chx’s picture

So... you guys decided that what sun did was , basically, OK? Fine... drawing the conclusions necessary.

webchick’s picture

I don't have enough context to know what sun did or didn't do, nor do I explicitly condone or un-condone it. I'm focused on technical solutions here, which is what the issue queue is for. We need a path forward on the #1 critical blocker for CMI (the synchronization feature), and the path laid out by heyrocker is a reasonable technical path forward.

If you guys have inter-personal conflict, please work it out between yourselves.

gdd’s picture

I am not saying that anything anyone in this issue did was or wasn't OK. What I'm saying is

1) This issue blocks another big issue
2) This issue has been committed
3) It is clear at this point that the patch will not be rolled back
4) Lets focus on the big picture and move forward, because we need to get these architectural issues in SOON or we're screwed.

chx’s picture

OK, so already last week #1674060: We need a new Freenode group contact and #1668716-5: How to deal with entity properties? is my stance on everything Drupal 8.

sun’s picture

Copied the missing parts into #1670370: Enforce writing of config changes during import, even if the module callback handled it already

Moved #86 into an own issue:
#1677258: Configuration system does not account for API version compatibility between modules

Lastly, as promised, I provided new extracted patches for all the other major config system issues already. Reviews welcome!

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.