Hello guys.

I've used this module for a moment, and I've noticed there isn't config/schema file for the provided Views style plugin, so, validations with config_inspector will fail.

Patch on the way...

CommentFileSizeAuthor
#9 Missing_config_schema-3232408-9.patch2.19 KBjordik
#3 config_schema-3232408-3.patch3.35 KBkuntyi
#2 config_schema-3232408-2.patch3.35 KBwaspper

Comments

waspper created an issue. See original summary.

waspper’s picture

waspper commented
StatusFileSize
new config_schema-3232408-2.patch3.35 KB

Patch here. Initial approach. Please, review/improve as needed.

kuntyi’s picture

kuntyi commented
StatusFileSize
new config_schema-3232408-3.patch3.35 KB
tr’s picture

tr commented

@Kuntyi When you post a new patch please say why you are posting the patch - is something wrong with the initial patch by @waspper?

Likewise, always post an interdiff.txt so we can see what the differences between your patch and the previous patch are - that will make it easier to evaluate your changes.

In this case, as far as I can tell, #3 has simply changed some datatypes from boolean to integer. That appears to be a wrong change - the core ViewsTable style plugin, from which ours is derived, uses boolean for those variables, and from the description boolean appears to be the proper type. If they're not being treated as booleans within views_aggregator that's a bug in views_aggregator which should be fixed. Changing the schema to use the wrong datatype is not a solution.

Can't we just extend the existing base type of views.style.table by doing something as simple as 

views.style.views_aggregator_plugin_style_table:
  type: views.style.table
  label: 'Table with aggregation options'
  # aggregation-specific stuff gets added here ...
  # everything else is inherited from parent schema

instead of copying over the entire definition for views.style.table and trying to keep our version of it current against changes in core?

waspper’s picture

waspper commented

Hello guys :)

Well, about using "views.style.table" as type, that was my initial idea/attempt. Then I saw some items that shoulnd't be there (I cannot remember right now), and I decided to use own schema. But for sure, we can to decide best approach. No problem at my side.

waspper’s picture

waspper commented
Status: Active » Needs review
tr’s picture

tr commented

"I saw some items that shoulnd't be there"

If you can identify those that would be great. The problem with extending the core table style is that it's easy to drift away from what core does unless we pay very close attention to changes to that core code and port all those changes to this module on an ongoing basis. We should use opportunities like this (adding a schema) to examine our table style and ensure it is up-to-date. If the change I suggested in #4 doesn't fully work, that's an indication that we need to also change something else in our code.

tr’s picture

tr commented
Status: Needs review » Needs work
jordik’s picture

jordik commented
Status: Needs work » Needs review
StatusFileSize
new Missing_config_schema-3232408-9.patch2.19 KB

This is the updated schema file using TR's proposal in #4 and the patch from #2 as base.
We extend views.style.table and add the keys we need for views.style.views_aggregator_plugin_style_table.

Thank you for the contributions, ready to be committed.

  • JordiK committed 1410704 on 8.x-1.x 
    Issue #3232408 by waspper, JordiK, TR: Missing config/schema
jordik’s picture

jordik commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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

jordik’s picture

jordik commented

Was reverted due to #3260887: Performance issues - timeout and memory errors, needs to be committed again.