Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
There is no need to a separate SchemaStorage class to exist.
Proposed resolution
Refactor base schema types to be read in TypedConfigManager and refactor other functionality into InstallStorage and ExtensionInstallStorage.
The main benefit is we have less code to maintain and more explicit assumptions in the correct place.
Remaining tasks
Review
User interface changes
None
API changes
Remove SchemaStorage
Comment | File | Size | Author |
---|---|---|---|
#15 | 2184387.15.patch | 10.83 KB | alexpott |
#15 | 14-15-interdiff.txt | 1.39 KB | alexpott |
#14 | 2184387.14.patch | 10.83 KB | alexpott |
#14 | 10-14-interdiff.txt | 2.72 KB | alexpott |
#10 | interdiff.txt | 2.54 KB | sun |
Comments
Comment #1
alexpottA patch to review :)
Comment #3
alexpottTalked with @sun in IRC and he came up with the idea that maybe core should have a config/schema directory.
Also fixed up the strange LocaleConfigManagerTest that was making unnecessary use of
InstallStorage
Comment #4
sunCan a config/schema directory have subdirectories?
Thus far, I assumed we'd only support files in that directory, not further subdirectories?
In other words, I expected this to simply be:
?
Comment #5
sunIf we'd additionally do the change in attached patch, then we could simply use
getComponentNames('core', 'core')
as is? :)Comment #6
sunThe proposal in #5 is also part of the architectural proposal in the new extension system meta.
Comment #7
sunCreated concrete proposal + patch for #5 now:
#2228921: Consolidate system.module + system.theme + system.theme.disabled into core.extension config
Comment #8
sunThat change has landed, so this patch gets much simpler now :-)
Re-rolled against HEAD, and:
Comment #10
sunRemoved obsolete TestSchemaStorage.
Comment #11
sun10: schema.storage.10.patch queued for re-testing.
Comment #12
alexpottRemove the word controller it is confusing with route controllers.
Can we not just call $collection directory. You might have plans to move this to key value but atm it is the file system.
Comment #13
BerdirLooks great, I just found two minor coding style issues. Certainly makes sense, it's a somewhat similar pattern as the language override storage, that also makes storage folder configurable and even allows to change it.
Agree with $collection being a strange name, +1 to directory. As those storages are explicitly about scanning module config directories, they will never be related to key value collections...
Also +1 to to remove controller, we already have a separate issue to remove it everywhere but when we touch it anyway...
Can we fix the line her as well? Should be something like Constructs a X object, we can't do @inheritdoc if we extend it.
@inheritdoc.
getAllFolders() is a very confusing method name, assumed that it returns folders, but it returns an array keyed by configuration files with the folder as value.
Not related to this issue.
Just wondering if there's an actual change on those lines or just making them better readable? Because I don't see a change :)
Comment #14
alexpottFixed everything from my review.
And for @berdir
Comment #15
alexpottUsing directory consistently...
Comment #16
BerdirThanks, this looks good now I think.
@alexpott and I shortly discussed folder vs directory and it looks like folder is more used as an UI term in windows/osx and the only place where we used folder right now, is the getAllFolders() method and the related $this->folders property, for which @alexpott already opened an issue to rename it to something better.
I assume this will pass, so RTBC.
Comment #17
catchCommitted/pushed to 8.x, thanks!