Goal

  • Research and discuss the required and expected functionality and behavior for the storage manager; e.g.:
    • Behavior when multiple storages are available, which allow a certain [read|write] access operation. (execute operation on both?)
    • Minimum requirements for instantiation/construction. (e.g., at least 1 storage? And/or at least 1 readable storage?)
    • Error handling. (e.g., attempting to write config when no writable storage is available)
  • Implement the regular runtime storage manager and a better/proper storage selection logic.
  • Discuss and potentially implement a second storage manager for performing imports/exports/synchronizations of configuration. (i.e., the original purpose of the writeToActive(), isOutOfSync(), etc methods on (some of) the storage controllers, which were not actually used anywhere)

System requirements

Using W3C/RFC language syntax; don't be scared by uppercase terminology. ;)

  1. Regular runtime code MUST NOT refer to specific storage controller classes directly. Only test code MAY do that. That is, because the active store MUST be pluggable, the passive store MAY be pluggable.
  2. Import/install-related code MUST be able to lock read and/or write access to a storage. At minimum, read access to the source storage MUST be locked during an import.
  3. Edge-case environments, such as install.php and update.php, MUST be able to operate on a read-only, file-based storage only (even as active store until a viable storage engine exists).
  4. Early bootstrap SHOULD be able to operate on file-based configuration, if desired. That is, in order to skip loading of larger subsystems on high-performance/large-scale sites.
  5. The system SHOULD be able to write to the active store and file storage for every write operation, if enabled. That is, to aid developers working on local configuration changes that will be commited, pushed, and staged upstream afterwards.
Files: 
CommentFileSizeAuthor
#4 1624580-config_simplified_storage.patch19.67 KBJose Reyero
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1624580-config_simplified_storage.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

heyrocker’s picture

Title: Rename DrupalConfig to StorageManager, and research and implement a proper design » Research and implement a proper design for StorageManager

I tend to agree that the rename should happen in the other issue, so we should limit this one to architecture questions.

Jose Reyero’s picture

FileSize
19.67 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1624580-config_simplified_storage.patch. Unable to apply patch. See the log in the details link for more information. View

This is an alternate, really simplified implementation for configuration storage. Storage objects are simple, do just one task, are completely reusable and can be passed around for different operations.

- DatabaseStorage and FileStorage just do the simple task of reading / writing to db / files.
- The configuration name is kept in the Config object and passed to storage classes.
- The import / export, or other logic lives now in a ImportStorage class (which implements StorageInterface and basically reads and writes to/from different sources).

Jose Reyero’s picture

I think you better don't looks at this patch but at the newest version, which is added here and includes the config rework patch, #1605324: Configuration system cleanup and rearchitecture

(Which is basically the same as here but instead of StorageManager implementing StorageInterface, it provides a getSource() and getDestination() methods to retrieve each of the Storages)

Basically it looks like:

class StorageManager {
 public function __construct(StorageInterface $source, StorageInterface $destination = NULL) {
  /**
   * Returns a storage controller to use as a source (read operations).
   */
  public function getSource()
  /**
   * Returns a storage controller to use as a source (read operations).
   */
  public function getDestination()
  /**
   * Copy configuration object from source to destination.
   */
  public function copy($name)
  /**
   * Copy all configuration objects from source to destination.
   */
  public function import()
  /**
   * Delete all configuration objects from source to destination.
   */
  public function uninstall()
}
sun’s picture

I think we should really have a discussion about expectations first. This entire storage manager/dispatcher logic has never been discussed in detail, since it always was implicit built-in functionality of the current DrupalConfig object. In other words, the current code just does what it does, but we never thought through what it is supposed to be capable of doing (and what it shouldn't).

For example, some expectations we could have, and which are changing things fundamentally, are:

  1. If configured to do so, every save operation on a config object writes to the active store AND the file storage.

    Enabling this option simplifies the development process on local developer sites, on which configuration changes will be checked into the file versioning system (e.g., git) afterwards, so configuration does not have to be explicitly exported first. By default, the system writes to one storage (the active store) only.

  2. The active store is pluggable and thus not necessarily DatabaseStorage. No code in our code-base should explicitly use or reference DatabaseStorage.

    @see #1605338: Implement pluggable storage for configuration system

  3. High-performance large-scale sites using advanced (page) caching methods along with alternative NoSQL cache backends have to be able to load early bootstrap configuration being required for delivering cached responses without loading the Database subsystem.

    @see #1576322-27: page_cache_without_database doesn't return cached pages without the database

  4. The Drupal installer needs to be able to read file-based configuration for a specific distribution, as well as a specific installation profile (as soon as selected), before any database or active store exists.

I've put them into quotes, as I'm not saying that we have them, but only that I could reasonably see us saying that we have them.

(In fact, I'd personally say we that we have these expectations. But alternative perspectives are possible.)

There are quite potentially more. And let's also not forget the very basic starting hints in the issue summary.

Jose Reyero’s picture

Well, it looks like we are thinking about the same, all that you say above, which is, in short some abstracted 'storage' concept that tells where to write and where to read from, right?

Actually that is what my patch implements. The only difference I see from this implementation and the others I've seen in other patches (call it storage manager, dispatcher, whatever) is that in this patch:
- It is simpler.
- It has more encapsulation and portability as an storage method (implements an StorageInterface) is an actual object you instantiate once and you can pass around, or use a different one for a different purpose.

Moreover, the StorageManager I'm thinking of allows encapsulating export and import operations and is actually used in other places of the code, like for importing the configuration (thus it proves to be both reusable and flexible).

sun’s picture

Title: Research and implement a proper design for StorageManager » Research and implement a proper design for StorageDispatcher

I'm trying to understand, but arguments like "it is simpler" don't really help ;)

If we would assume that #6 are our technical requirements (there's no discussion or agreement on those yet though), then how would those be supported and implemented?

FWIW, already with 1) I'm not sure I'm able to see how a dispatcher that takes a single source and a single target storage is able to handle two targets?


I'm going to move the list from #6 into the summary for now. In general, however, I strongly believe that we have to discuss our technical requirements and expectations first.

Because, if it would turn out that all points in that list are moot (unlikely, but possible), then we could as well remove the StorageDispatcher and its business logic entirely. That's the opposite end of possible directions. And because it is possible to take that direction, it's important to have the StorageDispatcher untangled from the rest so we can see it and properly discuss it.

Jose Reyero’s picture

> FWIW, already with 1) I'm not sure I'm able to see how a dispatcher that takes a single source and a single target storage is able to handle two targets?

Well, give it two targets, there's no reason why you cannot implement an storage method (StorageIngerface) that writes to two targets instead of one, the StorageManager will stay the same.

(You are not thinking of moving that logic into the config object, are you?)

Really, this patch implements one example of that and *actually uses it* for something (import/uninstall), which is way more than any other code example I've seen. Because for just replacing StorageInterface for the config object we don't need anything more complex than StorageInterface.


class MyFantasyStorage implements Storage Interface {
   /**
    * Add one more target to a list of targets to write sequentially.
    */
   function addTarget(StorageInterface $s) {
   }
}

$fantasy = new MyFantasyStorage();
$fantasy->addTarget(new DatabaseStorage());
$fantasy->addTarget(new Filestorage(...));
$fantasy->addTarget(new RemoteConfigBackupStorage());

$storage_manager_that_can_write_to_many_sources = new StorageManager($fantasy);

// And we're all set.....

// .... however if we want more, we can make it the default for the config system to write.
drupal_container->set('config.storage',  $storage_manager_that_can_write_to_many_sources);

// Or we can create a config factory that retrieves config objects that will write to all that sources and doesn't need to be the same as the default one that is into the container.

$factory = new ConfigFactory($storage_manager_that_can_write_to_many_sources);

Now please if we can have a list of requirements I'll show you how such architecture can fullfill all of them.

RobLoach’s picture

Not saying I endorse it, but this is related: #1665022: Rename StorageDispatcher to StorageArbiter.

Jose Reyero’s picture

Following up from the stimulating discussion here, #1605324: Configuration system cleanup and rearchitecture

I really think we should consider first of all whether we need a StorageDispatcher/Manager/Arbitrer/Whatever at all.

Jose Reyero’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

I've started to write down a concrete list of technical requirements in the summary. The list is not complete, only a start.

Please note that I'm personally still very open-minded on StorageDispatcher "to be or not to be" and always have been. I'm not married to any of the current code. In fact, I already considered and actually approached to remove it entirely myself (although I ran into systematic problems each time). Thus, there's no need to "defend" or strongly argue for a certain proposal or solution. We're all trying to figure out 1) what our actual needs are and 2) what we need to implement to fulfill our needs.

As mentioned many times in other issues, the actual theoretical concept behind this facility was never really discussed to great lengths of CMI sprints/discussions in the past. Therefore, I'd personally prefer to figure out a plan first. A solid plan depends on a definite list of requirements. The requirements are not 100% clear yet. So, before attempting to put anything into code, we need to know our actual requirements - so we can start to shape a concrete plan.

Jose Reyero’s picture

@sun
> So, before attempting to put anything into code, we need to know our actual requirements

Right though since somehow the code for StorageDispatcher already got in I think we are really in a hurry to define this.

Jose Reyero’s picture

Well, this is my design for StorageDispatcher, living proof of us not needing it, #1671080: Remove StorageDispatcher to simplify configuration system

sun’s picture

So we had a conf call today, mainly about multilingual configuration + overrides + realms (#1646580: Implement Config Events and Listeners, and storage realms for localized configuration), and discussed that we basically don't need the StorageDispatcher anymore.

The idea is to remove it entirely and just simply have:

  $active_storage = drupal_container()->get('config.active');
  $file_storage = drupal_container()->get('config.passive'); // file vs. passive TBD.

As a separate follow-up, we want to re-introduce the functionality of the current StorageDispatcher as a MultiStorage controller into core. The Config\StorageInterface defines that every storage controller takes $options for how to operate, and essentially, the MultiStorage controller will take the storage controller information that the current StorageDispatcher takes currently. In essence, the MultiStorage controller will read/list from a particular storage, and write/delete to multiple storages. Its business logic is limited to CRUD operations.

To confirm this direction, let's iterate over the known requirements in the issue summary:

Regular runtime code MUST NOT refer to specific storage controller classes directly. Only test code MAY do that. That is, because the active store MUST be pluggable, the passive store MAY be pluggable.

ACK. All code will retrieve the right storage controller through the Container.

Import/install-related code MUST be able to lock read and/or write access to a storage. At minimum, read access to the source storage MUST be locked during an import.

NACK. Actually, this requirement seems to be obsolete, since #1609760: hook_image_style_*() is not invoked for image styles upon Image module installation implemented the config import mechanism in a way that it directly writes to the active storage.

Furthermore, the import mechanism builds a "fake" config object for the source (file) storage already, by instantiating a new Config(NULL) and injecting the source storage data into it. Thus, even if a module would save that object, it would be written to the target/active store, not to the source/file storage.

Edge-case environments, such as install.php and update.php, MUST be able to operate on a read-only, file-based storage only (even as active store until a viable storage engine exists).

ACK. There's not really a dependency on the StorageDispatcher for this in the first place. And it should probably be noted that the current DatabaseStorage controller is able to work at the very first Drupal bootstrap phase/level already.

Early bootstrap SHOULD be able to operate on file-based configuration, if desired. That is, in order to skip loading of larger subsystems on high-performance/large-scale sites.

ACK. That's very similar to the previous point. In the worst possible implementation approach, we'd simply swap out the service definition on the Container after reaching a higher bootstrap phase/level.

The system SHOULD be able to write to the active store and file storage for every write operation, if enabled. That is, to aid developers working on local configuration changes that will be commited, pushed, and staged upstream afterwards.

ACK. The MultiStorage controller will allow that. Developers can enable it instead of the default implementation.

Jose Reyero’s picture

You know I'm all for dropping that StorageDispatcher. Though I don't understand why do we need this 'passive' and 'acvive' (unless you tell me they are used anywhere atm). So I would go with a simple:

$storage = drupal_container()->get('config.storage');
sun’s picture

Status: Active » Needs review

The import/export mechanism and other areas need direct access to the "passive" (file) storage, and I think they should retrieve the service definition instead of hard-coding FileStorage.

Status: Needs review » Needs work

The last submitted patch, 1624580-config_simplified_storage.patch, failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Needs review

I assume that status change was a mistake? The patch isn't passing tests, and it looks like this is still in active discussion.

sun’s picture

Status: Needs review » Closed (won't fix)

I guess the proper status is won't fix at this point, since #15 clarifies that we actually want to #1671080: Remove StorageDispatcher to simplify configuration system.

sun’s picture

Issue summary: View changes

Updated issue summary.