Problem/Motivation

The baloney.spam condition plugin provides no configuration schema, and its name should not contain a dot.

Proposed resolution

  • Rename the plugin ID to plugin_baloney.
  • Provide a configuration schema for the plugin.
  • Provide a fallback schema for condition plugins that have no configuration.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug, because its a missing config schema on tests, so some tests cannot be written.
Issue priority Normal, because it doesn't affect anything on runtime.
Disruption No disruption, just test changes.
CommentFileSizeAuthor
#18 drupal_2547581_18.patch2.82 KBxano
#18 interdiff.txt400 bytesxano
#10 drupal_2547581_10.patch3.21 KBxano
#10 interdiff.txt1.61 KBxano
#6 drupal_2547581_6.patch1.59 KBxano
#6 interdiff.txt392 bytesxano
#2 drupal_2547581_1.patch1.59 KBxano

Comments

Xano created an issue. See original summary.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Status: Active » Needs review
StatusFileSize
new drupal_2547581_1.patch1.59 KB

These bugs were discovered due to test failures in #2537732: PluginFormInterface must have access to the complete $form_state (introduce SubFormState for embedded forms). I'm not sure why this hasn't failed before, or how this should be tested.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
+++ b/core/modules/system/config/schema/system.schema.yml
@@ -347,3 +347,6 @@ condition.plugin.request_path:
+
+condition.plugin.*:
+  type: ignore

remove this again?

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

Why? It's a bit silly to require plugins without configuration to provide a schema.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

it's pretty common practice, because it forces them to think about it.

If anything, then make it type: mapping, so they need to do it if they have config. But ignore ignores everything.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
StatusFileSize
new interdiff.txt392 bytes
new drupal_2547581_6.patch1.59 KB

The last submitted patch, 2: drupal_2547581_1.patch, failed testing.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

That was the first patch, for which I cancelled testing.

Status: Needs review » Needs work

The last submitted patch, 6: drupal_2547581_6.patch, failed testing.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1.61 KB
new drupal_2547581_10.patch3.21 KB
xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Title: Missing configuration schemas for plugins » Missing configuration schemas for condition plugins
Assigned: xano » Unassigned
Priority: Normal » Critical

Bumping this to critical, since #2528178: Provide an upgrade path for blocks context IDs #2354889 (context manager) was a critical and this issue adds what that one should have added.

dawehner’s picture

dawehner
Primary language German
commented
Priority: Critical » Normal

This is a test, I can't see on earth why this is critical, sorry.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

Exactly. It's a test. If we can't trust those, then how do we know if the other issue was successfully fixed?

Anyway, I trust you can judge this better than I can. See you in the sprint room for coffee? ;-)

dawehner’s picture

dawehner
Primary language German
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

I think the patch itself is alright.

+++ b/core/modules/system/config/schema/system.schema.yml
@@ -347,3 +347,6 @@ condition.plugin.request_path:
+
+condition.plugin.*:
+  type: mapping

This seems to be a little bit out of scope to just fix the issue, but having it, can't be bad.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/system/config/schema/system.schema.yml
@@ -347,3 +347,6 @@ condition.plugin.request_path:
+
+condition.plugin.*:
+  type: mapping

This already exists...

condition.plugin:
  type: mapping
  label: 'Condition'
  mapping:
    id:
      type: string
      label: 'ID'
    negate:
      type: boolean
      label: 'Negate'
    uuid:
      type: string
      label: 'UUID'
    context_mapping:
      type: sequence
      label: 'Context assignments'
      sequence:
        type: string
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
+++ b/core/modules/block/tests/modules/block_test/config/schema/block_test.schema.yml
@@ -5,3 +5,6 @@ block.settings.test_block_instantiation:
+condition.plugin.baloney_spam:
+  type: condition.plugin

And this is not even using the condition.plugin.* added by the patch. So I guess the condition.plugin.* definition was left in by mistake.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

condition.plugin.* was added as an intended fallback for plugins not providing their own config schemas. Was I wrong in assuming condition.plugin cannot be used as a fallback because it contains no asterisk?

What does condition.plugin even describe? It's not defined as a config entity schema, but it contains a UUID and other schemas for configuration that does not show up in ConditionPluginBase.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt400 bytes
new drupal_2547581_18.patch2.82 KB

Several reviewers found the fallback schema redundant, so I removed it.

condition.plugin is a mess anyway (unnecessarily merged plugin ID and plugin configuration, schemas for non-existing properties), so let's clean that up in a follow-up.

Xano queued 18: drupal_2547581_18.patch for re-testing.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Thank you for keeping the patch in scope!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • webchick committed ad22ba7 on 8.0.x 
    Issue #2547581 by Xano, dawehner, Berdir, alexpott: Missing...

Status: Fixed » Closed (fixed)

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