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.

Comments

axel.rutz created an issue. See original summary.

caldenjacobs’s picture

Priority: Major » Critical
pasqualle’s picture

Title: Revisit config » Split config

1: 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:

node_type_blog_status: '2'
node_type_blog_pattern: ''
node_type_blog_php: 0
node_type_faq_status: '1'
node_type_faq_pattern: '[node:field_question]'
node_type_faq_php: 0

This should be split to:
auto_entitylabel.settings.node.blog.yml

dependencies:
  config:
    - node.type.blog
status: 2
pattern: ''
php: false

auto_entitylabel.settings.node.faq.yml

dependencies:
  config:
    - node.type.faq
status: 1
pattern: '[node:field_question]'
php: false
pookmish’s picture

Status: Active » Needs review
StatusFileSize
new9.29 KB

The 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.

pookmish’s picture

Rerolled, to fix the dependency on each config.

guptahemant’s picture

Assigned: Unassigned » guptahemant
Status: Needs review » Needs work
StatusFileSize
new58.71 KB
new45.78 KB

hi @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.

pookmish’s picture

@guptahemant, If you are satisfied with that approach, I have a patch that includes an update hook.

pookmish’s picture

StatusFileSize
new12.51 KB

Heres an updated patch with update hook & inclusion of the new `escape` setting.

pookmish’s picture

Status: Needs work » Needs review
guptahemant’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new107.43 KB
new58.71 KB
new45.78 KB

Hi @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.

purushotam.rai’s picture

purushotam.rai’s picture

This seems something very interesting and is definitely needed. Quite impressive work done by you guys.

Thanks for your valuable inputs.

purushotam.rai’s picture

Status: Reviewed & tested by the community » Fixed
geek-merlin’s picture

Status: Fixed » Active

Hey 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.

pookmish’s picture

@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.

pookmish’s picture

Status: Active » Fixed
geek-merlin’s picture

Aah great, in fact i only reviewed the patch... Cool!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.