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.
Comment | File | Size | Author |
---|---|---|---|
#98 | interdiff.txt | 870 bytes | chx |
#96 | drupal8.config-import-module-api-test.96.patch | 5.86 KB | sun |
#96 | interdiff.txt | 870 bytes | sun |
#95 | 1609760_95.patch | 5.59 KB | chx |
#78 | drupal8.config-import-module-api-test.2.patch | 1.8 KB | sun |
Comments
Comment #1
sunNote: 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:
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.Comment #2
catchI 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.
Comment #3
sunYes, "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 fromDrupalConfig
(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 aDrupalConfig
object, runtime code will act on aConfigObject
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:
For the latter (global definition), I'm not sure how it would look.
Comment #4
catchMarked #1614274: Code in hook_config_sync() implementations duplicates module API functions as duplicate.
Comment #5
chx CreditAttribution: chx commentedNow 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.
Comment #6
chx CreditAttribution: chx commentedMeh!
Comment #7
chx CreditAttribution: chx commentedNote: 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.
Comment #9
chx CreditAttribution: chx commentedSorry bot.
Comment #10
chx CreditAttribution: chx commentedComment #12
Anonymous (not verified) CreditAttribution: Anonymous commentedi 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().
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedwoops. bot cross posted on me.
Comment #14
catchJust 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.
Comment #15
chx CreditAttribution: chx commentedThis 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.
Comment #16
catchI 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.
Comment #17
sunThe 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:
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.
Comment #18
Anonymous (not verified) CreditAttribution: Anonymous commented"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 ;-)
Comment #19
sunWhat I'm saying is:
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:
And if any, use that instead of the pure/plain config save operation:
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:
image.style.foo == image_style.foo
).Comment #20
Anonymous (not verified) CreditAttribution: Anonymous commented"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.
Comment #21
chx CreditAttribution: chx commentedThe 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).Comment #22
chx CreditAttribution: chx commentedOh! 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.
Comment #23
sunThe 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:
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.
Comment #24
chx CreditAttribution: chx commentedNo 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.
Comment #25
chx CreditAttribution: chx commentedThis 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.
Comment #26
chx CreditAttribution: chx commentedThis one fixes the is_new / isNew property mixup I had in #25.
Comment #28
chx CreditAttribution: chx commentedThe name of hooks start with config_presave__ not with config_presave_ .
Comment #29
chx CreditAttribution: chx commentedNow with much less debug, I found even #25-#26 had debug in it.
Comment #30
sunIt 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:
What I'm saying is:
Comment #31
chx CreditAttribution: chx commentedI 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.
Comment #32
catchHow'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.
Comment #33
sun...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:
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:
...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.
Long story short: You can update the {image_styles} table directly. But is it correct to do so? Certainly not.
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.
"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.
Comment #34
chx CreditAttribution: chx commentedHrm? We first run
hook_config_presave
, thenhook_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.Comment #35
sunFYI: 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.Comment #36
gddI 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:
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
Comment #37
chx CreditAttribution: chx commentedSo 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.
Comment #38
Anonymous (not verified) CreditAttribution: Anonymous commentedafter further discussion in IRC, i'm coming around to the position that we shouldn't have a hook fire on $config->save().
Comment #39
merlinofchaos CreditAttribution: merlinofchaos commentedchx: 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.
Comment #40
catchNot sure I get this bit:
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?
Comment #41
sun#1447686-129: Allow importing and synchronizing configuration when files are updated now contains a patch that turns hook_config_sync() into "change dispatcher" instead.
$view = new View($new_config);
).Comment #42
chx CreditAttribution: chx commentedWhat #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.
Comment #43
fagoI 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.
Comment #44
gddAssuming 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?
Comment #45
sunYes. 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.
Comment #46
catchSorry 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.
Comment #47
sunAh. 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.:
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. :)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.foo.bar.baz
belongs to Foo module.MODULE_config_import()
. If the module implements this callback, then it is asked to process the configuration data on its own: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.
Comment #48
sunActually, 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.
Comment #49
sunThat 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.
Comment #50
gddI 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.
Comment #51
sunThanks! :)
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.
Comment #52
webchickGoodness, 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:
80 chars. Can probably omit "an", "a", etc. and get it to fit.
..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?
No comma.
Elsewhere the API does "old, new" so I think we should swap the order of these parameters. Maybe?
Should we make two hooks then? Copy/pasting this check in every hook implementation seems ugh.
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.
We can probably remove this @todo code from the sample code. Else change it to @todo: update based on changes in image_config_import().
Comment #53
sun1) 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.
Comment #54
webchickDiscussing 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.
Comment #55
sunFixed 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.
Comment #56
webchickOk, looks like that addresses my feedback; thanks!
Committed and pushed to 8.x! Thanks a lot.
Comment #57
chx CreditAttribution: chx commentedI 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
Comment #58
chx CreditAttribution: chx commentedSo this issue is basically #1447686: Allow importing and synchronizing configuration when files are updated rehashed.
Comment #59
chx CreditAttribution: chx commentedAnd 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.Comment #60
sunIn 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.
Comment #61
chx CreditAttribution: chx commentedIt 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.
Comment #62
chx CreditAttribution: chx commentedSeems I make more sense when I talk in PHP.
Comment #63
sunThe 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.
Comment #64
chx CreditAttribution: chx commentedThe 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.
Comment #65
chx CreditAttribution: chx commentedNote 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.
Comment #66
gddhook_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.
Comment #67
chx CreditAttribution: chx commentedIf there is no usecase for bypassing CMI then why my vastly simplified patch is not considered?
Comment #68
chx CreditAttribution: chx commentedNote: 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.
Comment #69
sunIn 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.
Comment #70
Anonymous (not verified) CreditAttribution: Anonymous commentedre #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?
Comment #71
xjm@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.
Comment #72
chx CreditAttribution: chx commentedI can quote the DCOC too!
Comment #73
xjmYep, I think that also applies. :)
Comment #74
chx CreditAttribution: chx commentedNote: 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()...Comment #75
sunI'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:
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:
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.
Comment #76
chx CreditAttribution: chx commentedNope. 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.
Comment #77
sunSomeone 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
Goal
Details
Proposed solution
Counter arguments
Comment #78
sunAttached 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:
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:
This essentially repeats #132 and #136 from #1447686-132: Allow importing and synchronizing configuration when files are updated
Comment #79
chx CreditAttribution: chx commentedProblem: 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?
Comment #80
webchickWat? :(
Comment #81
chx CreditAttribution: chx commentedwat? 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.
Comment #82
sunIt 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.
Comment #83
chx CreditAttribution: chx commentedIt will not be rolled back. #62 is a patch to review, it does not involve architecture. The personal parts I am not reacting to.
Comment #84
chx CreditAttribution: chx commentedAnd, the work I am grateful for. Really.
Comment #85
sun#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:
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:
If you don't want to use the module's API, then you better don't use the module.
Comment #86
sunOn 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.
Comment #87
Anonymous (not verified) CreditAttribution: Anonymous commentedhad a long discussion with sun and chx about #70.
to try to summarise:
Comment #88
chx CreditAttribution: chx commented"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.
Comment #89
sunParaphrased and anonymized third-person transcript from my IRC discussion with @beejeebus:
Comment #90
chx CreditAttribution: chx commentedOK 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):
Comment #91
sunAfter 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.
Comment #92
sunEssentially, if we trust module APIs to do the right things, then the patch in #78 should be marked RTBC - it testifies the expectation.
Comment #93
sunThis 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.
Comment #94
chx CreditAttribution: chx commentedIt'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.
Comment #95
chx CreditAttribution: chx commentedHere'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.
Comment #96
sunTo prove that #62 contradicts #78.
Comment #98
chx CreditAttribution: chx commentedYou 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.
Comment #99
gddI 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.
Comment #100
gddTo 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.
Comment #101
webchickI 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.
Comment #102
chx CreditAttribution: chx commentedSo... you guys decided that what sun did was , basically, OK? Fine... drawing the conclusions necessary.
Comment #103
webchickI 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.
Comment #104
gddI 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.
Comment #105
chx CreditAttribution: chx commentedOK, 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.
Comment #106
sunCopied 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!
Comment #107.0
(not verified) CreditAttribution: commentedUpdated issue summary.