While the module just works fine, the config storage has imho some issues:
1) It contains bogus form keys
2) It has no schema
3) All config lives in auto_entitylabel.settings which torpedoes making features.
We should have separate config objects per bundle like auto_entitylabel.bundlesettings.node.mybundle
OR even better just use node.type.mybundle/third_party_settings.auto_entitylabel.
Setting high prio as this makes the module unusable with features or customized distributions.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | New single config.png | 45.78 KB | guptahemant |
| #10 | splitted cofig.png | 58.71 KB | guptahemant |
| #10 | old config.png | 107.43 KB | guptahemant |
| #8 | auto_entitylabel-split_configs-2876280-8.patch | 12.51 KB | pookmish |
| #6 | Screen Shot 2017-11-09 at 3.43.45 PM.png | 45.78 KB | guptahemant |
Comments
Comment #2
caldenjacobs commentedComment #3
pasqualle1: Form details were removed in #2878583: Module config includes form build details
2: Not sure what kind of schema should be here, please create a separate issue with details.
3. I agree, the config must be split into smaller pieces
- The default configuration is not easily included in custom modules (or features).
- Parts of the configuration are not deleted when dependent module/configuration is deleted.
example, current config (auto_entitylabel.settings.yml) looks like:
This should be split to:
auto_entitylabel.settings.node.blog.yml
auto_entitylabel.settings.node.faq.yml
Comment #4
pookmish commentedThe included patch splits the config from the single into separate configs. No upgrade path provided in this patch though. if this becomes an approved patch, i can investigate the upgrade path/legacy support.
Comment #5
pookmish commentedRerolled, to fix the dependency on each config.
Comment #6
guptahemant commentedhi @pookmish I tried your patch and here are the attached screenshots of how its working and splits the config ,Setting the status to needs work because I will be looking at the ways to apply the upgrade hook.
Comment #7
pookmish commented@guptahemant, If you are satisfied with that approach, I have a patch that includes an update hook.
Comment #8
pookmish commentedHeres an updated patch with update hook & inclusion of the new `escape` setting.
Comment #9
pookmish commentedComment #10
guptahemant commentedHi @pookmish,
I tested your new patch in #8,After testing the patch I found no issues and as can be seen in the screenshots all looks good and the module is functioning properly,It looks good to me.
I am marking the issue as RTBC.
Thanks for the great work.
Comment #11
purushotam.rai commentedComment #12
purushotam.rai commentedThis seems something very interesting and is definitely needed. Quite impressive work done by you guys.
Thanks for your valuable inputs.
Comment #14
purushotam.rai commentedComment #15
geek-merlinHey dudes, that's really big and fantastic work!
But alas, if i grok the code right, one important piece is missing:
> 2) It has no schema
Reopening thus.
Comment #16
pookmish commented@axel.rutz, it looks like the schema part was a duplicate of #2922980: Create schema for the new configuration objects generated in the parent issue and so looks like the schema file was added.
Comment #17
pookmish commentedComment #18
geek-merlinAah great, in fact i only reviewed the patch... Cool!