Problem/Motivation

It is common practice for a module to provide a configuration schema when it makes use of configuration entities, and context currently does not. Read What are schema files used for? for reasons why including a schema is important.

Proposed resolution

Provide a schema that is broken down in the following way:
- General configuration schema
- Schemas for each condition
- Schemas for each reaction
- We're just going from loosely typed data to more strictly typed data. We may want to consider an update hook to load and resave all configs to force data to get properly typed.

CommentFileSizeAuthor
#22 interdiff-18-21.txt425 bytespaulocs
#21 context-2953766-21.patch4.22 KBpaulocs
#18 interdiff_17-18.txt2.55 KBdeaom
#18 context-2953766-18.patch4.17 KBdeaom
#17 interdiff_14-16.txt744 bytesdeaom
#17 context-provide_config_schema-2953766-17.patch2.78 KBdeaom
#14 interdiff_11-14.txt8.96 KBjaypan
#14 context-provide_config_schema-2953766-14.patch2.57 KBjaypan
#11 interdiff_2-11.txt2.56 KBdeaom
#11 context-provide_config_schema-2953766-11.patch7.54 KBdeaom
#3 context--2953766--configuration-schema-2.patch5.79 KBWidgetsBurritos

Comments

WidgetsBurritos created an issue. See original summary.

WidgetsBurritos’s picture

WidgetsBurritos commented
Status: Active » Needs work

This patch isn't complete but it's a start. The blocks conditions is a bit more complicated than the rest, so that will require some additional work.

Also I haven't included any sort of update hook yet to force a resave of the configuration. Not sure if that's desired or not. Wouldn't be too difficult to do.

WidgetsBurritos’s picture

WidgetsBurritos commented
StatusFileSize
new context--2953766--configuration-schema-2.patch5.79 KB

Whoops. Forgot to attach the file.

Chris Charlton’s picture

Chris Charlton
Primary language English
Location Los Angeles
commented

+1 for this support.

bighappyface’s picture

bighappyface commented
Status: Needs work » Needs review
Issue tags: +ContributionWeekend2019
bighappyface’s picture

bighappyface commented
Status: Needs review » Reviewed & tested by the community

#3 LGTM

* Confirmed new kernel test
* Confirmed use of schemas and validation of context configs

bighappyface’s picture

bighappyface commented

@boshtian may we have your help with review and/or merging?

boshtian’s picture

boshtian commented

I'll check this today and get back with feedback. Thank you for patience.

boshtian’s picture

boshtian commented

I checked this and most of it looks good, nice work WidgetsBurritos. But as he mentioned in the first comment, the Blocks reactions are missing from schema and I also noticed that Region reactions are missing.

If anyone is able to put in some additional work in this and I can review it later, that would be great. Otherwise I'll see if I can spend some time on this during this week and get back with the updated patch.

boshtian’s picture

boshtian commented
Status: Reviewed & tested by the community » Needs work
deaom’s picture

deaom commented
StatusFileSize
new context-provide_config_schema-2953766-11.patch7.54 KB
new interdiff_2-11.txt2.56 KB

Added the missing schema for block reactions and region reactions, patch is attached and also the interdiff.

deaom’s picture

deaom commented
Status: Needs work » Needs review

Changing the tag to needs review.

strozx’s picture

strozx commented
Status: Needs review » Reviewed & tested by the community

I have reviewed it and it works so moving this to "Reviewed".

jaypan’s picture

jaypan
Location Victoria, BC
commented
StatusFileSize
new context-provide_config_schema-2953766-14.patch2.57 KB
new interdiff_11-14.txt8.96 KB

Updating patch:

1) The plugin.reaction configuration data type needed to be defined in context.data_types.schema.yml, so that other modules can add reaction plugin configuration definitions.
2) The order of the definitions in context.schema.yml needs reordering - the reaction defintions need to come after the context plugin definition. Otherwise plugin definitions from other modules are not picked up.

jaypan’s picture

jaypan
Location Victoria, BC
commented
Related issues: +#3131921: Provide a Configuration Schema
paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Status: Reviewed & tested by the community » Needs review
deaom’s picture

deaom commented
Status: Needs review » Needs work
StatusFileSize
new context-provide_config_schema-2953766-17.patch2.78 KB
new interdiff_14-16.txt744 bytes

The updated patch has some missing schemas. (conditions.request_path_exclusion, reactions.blocks.blocks and reactions.blocks.saved). Attached is the patch for the conditions.request_path_exclusion and reactions.blocks.saved. I do not know how to add the reactions.blocks.blocks as my attempt from previous patch was not correct and any changes I made to schema trying to follow new patch logic it only gives me more missing schema errors. So if anyone has any ideas, please add the missing schema for the reactions.blocks.blocks. So marking this as needs work.

deaom’s picture

deaom commented
Status: Needs work » Needs review
StatusFileSize
new context-2953766-18.patch4.17 KB
new interdiff_17-18.txt2.55 KB

O.k., so I think I managed to add the reactions.blocks.blocks to schema. I also added the view_inclusion condition and corrected the include_default_blocks to integer instead of boolean. Somebody please check and test it.

Status: Needs review » Needs work

The last submitted patch, 18: context-2953766-18.patch, failed testing. View results

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Status: Needs work » Needs review

Set to needs review because no specific problems were found in the tests and it was build successfully.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
StatusFileSize
new context-2953766-21.patch4.22 KB

Patch #18 is almost there. I added the page_title reaction that was missing.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
StatusFileSize
new interdiff-18-21.txt425 bytes

Interdiff

Status: Needs review » Needs work

The last submitted patch, 21: context-2953766-21.patch, failed testing. View results

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Status: Needs work » Needs review

  • paulocs committed 246b98d on 8.x-4.x 
    Issue #2953766 by DeaOm, paulocs, Jaypan, WidgetsBurritos, bighappyface...
paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Status: Needs review » Fixed

Fixed in dev branch.

Thanks @all.

Status: Fixed » Closed (fixed)

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