Follow-up to #2183983: Find hidden configuration schema issues

Problem/Motivation

http://privatepaste.com/8b541b5cc4 has the list of configuration that are missing config schema.

sample

    [block.block.views_block__case_studies_block_1:settings.views_label] => Missing schema.
    [block.block.views_block__case_studies_block_1:settings.items_per_page] => Missing schema.
    [block.block.views_block__case_studies_block_1:settings.override] => Missing schema.
    [block.block.views_block__case_studies_block_1:settings.views_label_checkbox] => Missing schema.


    [core.base_field_override.comment.comment.changed:default_value] => Missing schema.
    [core.base_field_override.comment.comment.changed:settings] => Missing schema.

Proposed resolution

Remaining tasks

1. Add a test block with configuration that are missing schema.
2. Add missing schema and make patch green.
3.Review
4. Commit.

User interface changes

None.

API changes

Likely none.

Postponed until

#2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration

Comments

vijaycs85’s picture

Status: Active » Needs review
Issue tags: -needs test
FileSize
1.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,519 pass(es). View
1.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,530 pass(es), 4 fail(s), and 0 exception(s). View

Initial patch...

Status: Needs review » Needs work

The last submitted patch, 1: 2358263-views-config-schema-1-test-only.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review

let's set it for review. will update for fields as well.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Not sure putting the base field overrides and views blocks to one issue is good, are they related somehow?

+++ b/core/modules/views/config/schema/views.schema.yml
@@ -117,3 +117,24 @@ views.view.*:
+    views_label_checkbox:
+      type: boolean
+      label: 'Override title'

Is there an actual override value too or is that the regular block label then? In other words is this the full settings list of a views block or only this views block :)

Gábor Hojtsy’s picture

Title: Adding missing translation schema for block & field configuration » Adding missing configuration schema for views blocks

Retitling accordingly. Opening a new issue for base field overrides sounds sensible to me.

Gábor Hojtsy’s picture

The base field overrides problem is required to fix #2361615: Field type config schemas are not in the base schema too. Should be ideal to do there.

vijaycs85’s picture

Status: Needs work » Needs review

Alright. Still the patch in #1 is valid as the field part is out at #2361615: Field type config schemas are not in the base schema

Gábor Hojtsy’s picture

Do you have feedback on questions in #4?

vijaycs85’s picture

Is there an actual override value too or is that the regular block label then? In other words is this the full settings list of a views block or only this views block :)

As far as I can see, this is the only settings from views block.

dawehner’s picture

  1. +++ b/core/modules/config/tests/config_schema_test/config/install/block.block.views_block__content_recent_block_1_2.yml
    @@ -0,0 +1,29 @@
    +  items_per_page: none
    +  override:
    +    items_per_page: none
    
    +++ b/core/modules/views/config/schema/views.schema.yml
    @@ -117,3 +117,24 @@ views.view.*:
    +    items_per_page:
    +      type: string
    +      label: 'Items per block'
    +    override:
    +      type: mapping
    +      label: 'Override'
    +      mapping:
    +        items_per_page:
    +          type: string
    +          label: 'Items per block'
    

    This is even more confusing, why is both stored? Note: The block display determines the available options, see \Drupal\views\Plugin\views\display\Block::blockForm()so hard coding stuff here is probably not the best idea tbh.

  2. +++ b/core/modules/config/tests/config_schema_test/config/install/block.block.views_block__content_recent_block_1_2.yml
    @@ -0,0 +1,29 @@
    +  views_label_checkbox: true
    

    ... This is indeed a bug in ViewsBlockBase... We use this just to set up the views_label ... so we should unset it in ViewsBlockBase::BlockSubmit

vijaycs85’s picture

FileSize
2.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,610 pass(es), 1 fail(s), and 0 exception(s). View
2.38 KB

thanks for the review @dawehner.

#10.1 - agreed it is dynamic, but we are getting 'allow' option which is hard coded in Drupal\views\Plugin\views\display\Block::defineOptions. is there anyway to update, if so then we might need to extend block.settings.views_block:*? (e.g. block.settings.views_block:foo). Also unset the value to avoid duplicate.
#10.2 - Fixed.

Status: Needs review » Needs work

The last submitted patch, 11: 2358263-views-config-schema-11.patch, failed testing.

dawehner’s picture

#10.1 - agreed it is dynamic, but we are getting 'allow' option which is hard coded in Drupal\views\Plugin\views\display\Block::defineOptions. is there anyway to update, if so then we might need to extend block.settings.views_block:*? (e.g. block.settings.views_block:foo). Also unset the value to avoid duplicate.

Well, we could still make it somehow overridable using the plugin ID of the block display. Can we store that somehow in the views configuration?
Then we can leverage that in order to fetch the schema specific to the views display.

Gábor Hojtsy’s picture

@dawehner: is that still a concern with how the schema fits with current features or how features would ideally be? It would be great to fix the schema at least to correspond to the current features. Any concerns on that front?

xjm’s picture

Issue tags: +D8 upgrade path
vijaycs85’s picture

Status: Needs work » Needs review
FileSize
3.2 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,144 pass(es). View
522 bytes

Fix for test fails.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/config/schema/core.data_types.schema.yml
@@ -220,6 +220,11 @@ route:
+    entity:
+      type: sequence
+      label: 'Entity dependencies'
+      sequence:
+        - type: string
     config:
       type: sequence
       label: 'Configuration entity dependencies'

So these are content entity dependencies as opposed to config entities, no?

Gábor Hojtsy’s picture

As per https://www.drupal.org/node/2364725 there should be a config and a content key but not an entity key anymore.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
2.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,167 pass(es). View
1.15 KB

Thanks @Gábor Hojtsy for the reference. It has changed since the first export of this block.

Gábor Hojtsy’s picture

Superb. @dawehner: can you confirm that your concerns are with the patch or with how the code is to be improved generally (not in scope).

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Superb. @dawehner: can you confirm that your concerns are with the patch or with how the code is to be improved generally (not in scope).

Well, this was more of a theoretical concern, let's hope noone in contrib actually uses that functionality.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

I can't see how the new schema is being tested.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Ah I see - by the DefaultConfigTest

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/config/tests/config_schema_test/config/install/block.block.views_block__content_recent_block_1_2.yml
@@ -0,0 +1,26 @@
+  config:
+    - views.view.content_recent
+  module:
+    - views
+  theme:
+    - bartik

This introduces an unmet dependencies on the node, views and block. So this config never actually installed. So we're not really testing the current state of HEAD. We're testing the state of HEAD when this configuration was created. I think the test here should be better and test a views block created by a test.

vijaycs85’s picture

Issue tags: +Needs tests
Gábor Hojtsy’s picture

Status: Needs work » Closed (duplicate)
Issue tags: -sprint, -Needs tests

#2379697: Fix configuration schema issues in block content (indirectly link and field test) modules actually runs into the same problems so incorporating the fixes there. That creates a bunch of views blocks so strict schema checking in the test ensures they are tested (and exposes the fails there).