Problem/Motivation

Currently, logic for determining configuration storage differences is embedded in ConfigRevertController::generateReport(). Much of what's needed for this task looks to be available in StorageComparer:getChangelist(). Using that method, the code might be simpler and more consistent with core.

Proposed resolution

Either use StoragerComparer directly or, if necessary, extend it for customizations; see #2406059: Faciliatate customization of reports, e.g., distinguishing edits from extension changes.

Remaining tasks

User interface changes

API changes

Comments

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Thanks! I'll take a look at this.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Active » Closed (won't fix)

I took a look at this class as compared to what Config Revert is doing. There are quite a few differences:

* The purpose is different. StorageComparer is meant to be used during a bulk synch operation, to compare a stored snapshot to the active config. It is oriented towards trying to figure out what config changes need to be made, in what order (considering dependencies), not towards having a human being understand what has chagned.
* The sorting is different. StorageComparer sorts by dependencies (because it's made to figure out synch operation order), vs. alphabetical if a human being is trying to understand what has changed.
* It's oriented by operations that would need to be done to synch config, not by just seeing what has changed, and it's using UUID to decide this. This is not valid for comparisons between config/install config and active, but only for comparing snapshots to active.
* It does not do any normalization to decide whether config has changed or not, so it is going to have a lot of false positives due to the order of config arrays being different and the UUID being missing from config/install. Actually, given that UUID is missing, it will detect *everything* as being different. Oops.
* It uses config collections. We don't want them in Config Revert.
* It has no way to limit the config objects by type or module, and the interface this implements doesn't even have this concept.

So... I cannot really use this class, or even implement the interface, since it has a bunch of methods I don't need and lacks the ones I do need. Not a good match to what Config Revert needs to do.

nedjo’s picture

nedjo
he/him/his
Primary language English
commented
Version: » 8.x-1.x-dev

In ConfigSyncStorageComparer I've subclassed StorageComparer in a way that addresses at least some of these issues. It looks to me like the StorageComparer is usable for extensions, but not for all configuration of a given type

The purpose is different. ... It is oriented towards trying to figure out what config changes need to be made, in what order (considering dependencies)

Yes. However, a key interest - determining new, changed, removed, and renamed configuration items - is relevant. And analysis of dependency information may indeed be needed for importing or updating even a single item; see #2423211: Review config dependency handling in import operation.

The sorting is different.

Yes, any use for UI display would have to reorder results.

It does not do any normalization to decide whether config has changed or not

To determine updates, at least, we can address this in a subclass by overriding the addChangelistUpdate method to use ConfigDiffInterface::same() to determine difference. I haven't yet looked closely enough to see if other methods also need overriding.

It uses config collections. We don't want them in Config Revert.

Support for multiple collections isn't needed, but the default collection, StorageInterface::DEFAULT_COLLECTION, covers our use case.

It has no way to limit the config objects by type or module, and the interface this implements doesn't even have this concept.

Yes, this seems to be the key issue for Config Update Manager. For extensions, we can feed the extension's file storage as an argument in the constructor. I've roughed this in in the (draft, untested) code at ConfigSyncLister::getExtensionChangelist(). But for configuration types, it would be a much more involved process of calculating data for each extension and then extracting the type-specific data from all extensions. Given the expense of this operation, some sort of caching of state information would be needed. Opening #2425481: Store config override state.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Still not doing this.

dmezquia’s picture

dmezquia
Primary language Spanish
Location Spain
commented
Component: Code » Base module

Hi @jhodgdon i need to extend this class StorageComparer for my custom module(from de core), to customize the addChangelistDelete() method, do you know in addition to extending what do I need to do so that Drupal recognizes it? maybe define it as a service or something?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

I guess that depends on what you mean on "so that Drupal recognizes it". All you need to do to get the class to get auto-loaded is to put it in the correct subdirectory of your module with the correct namespace, and then use it in another class. But if you want Drupal itself to use it in place of StorageComparer, then that will be more difficult. You would need to override the service(s) that are using that class, and substitute your own.

If you need to do that, see
https://api.drupal.org/api/drupal/core%21core.api.php/group/container/8.8.x
for instructions.

dmezquia’s picture

dmezquia
Primary language Spanish
Location Spain
commented

Hi @jhodgdon thank por the answer, that exacly the problema i have, that this class i dont see it was a service, so this class its called as new StorageComparer in several parts of the code, so i think is not as a service.
Then there is not way to replaced and use my custom extendig class instead?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Right, you have understood correctly. Because Drupal Core didn't make that StorageComparer a service, it is not easy to override it!

So, in order to override that class you would need to find the classes and other places that use it and override them instead, if possible. Good luck. That doesn't look easy to do!

dmezquia’s picture

dmezquia
Primary language Spanish
Location Spain
commented

Thank you very much @jhodgdon you already clarified the question I had, well i opened an issue and created a patch here https://www.drupal.org/project/drupal/issues/3069522
that was what i did to solve the problem i had.