The current schema organization lets each widget/formatter plugin completely override the schema for $display->content[$field_name] (which is a keyed array with weight, plugin id, label and settings).
This is wrong and not in-line with the actual data model : the widget/formatter only gets to say what's inside 'settings', what's in the level above is not its decision.
This is also not consistent with the way field type plugins express the schema for their settings - field.[field_type].settings is *only* the mapping of settings.

API changes

This will force widget / formatter plugins to update the config schema for their settings.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Category: Task » Bug report

This looks like a bug rather than a task to me. I think it's OK to make the change at least until schemas settle down more generally on impact > disruption.

yched’s picture

adding tags

Gábor Hojtsy’s picture

Priority: Normal » Major
Status: Active » Needs review
Issue tags: +D8 upgrade path
FileSize
29.16 KB

There you go. @yched this is what you had in mind? We do get rid of loooooots of cruft in the schema also.

Status: Needs review » Needs work

The last submitted patch, 3: 2378055-refactor-entity-view-form-field-schemas.patch, failed testing.

Gábor Hojtsy’s picture

This fails on

PHP Notice:  Undefined property: Drupal\Tests\Component\EventDispatcher\ContainerAwareEventDispatcherTest::$results in /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh on line 650

Notice: Undefined property: Drupal\Tests\Component\EventDispatcher\ContainerAwareEventDispatcherTest::$results in /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh on line 650
Drupal\Tests\Component\EventDispatcher\ContainerAwareEventDi     passes
PHP Notice:  Undefined property: Drupal\Tests\Component\EventDispatcher\ContainerAwareEventDispatcherTest::$results in /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh on line 653

Notice: Undefined property: Drupal\Tests\Component\EventDispatcher\ContainerAwareEventDispatcherTest::$results in /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh on line 653
PHP Notice:  Undefined property: Drupal\Tests\Component\EventDispatcher\ContainerAwareEventDispatcherTest::$results in /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh on line 653

Notice: Undefined property: Drupal\Tests\Component\EventDispatcher\ContainerAwareEventDispatcherTest::$results in /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh on line 653

I don't see how this is related and cannot reproduce locally. Will work on fixing the other fails by attempting to reproduce them locally but does not sound very effective. #2370305: Refactor field type configuration schemas for DX, easier to find errors also fails on the same thing but is a totally different patch...

yched’s picture

@yched this is what you had in mind?

Way cool, yes, exactly !!
Well, if those fails can be solved :-)

Status: Needs work » Needs review
alexpott’s picture

Status: Needs review » Needs work

The last submitted patch, 3: 2378055-refactor-entity-view-form-field-schemas.patch, failed testing.

yched’s picture

So, the overall structure and mounting points are perfect.

My only nitpick is about schema entry names :

What #2370305: Refactor field type configuration schemas for DX, easier to find errors does is :
"field type plugins provide the config schema for their settings at field.(field|storage)_settings.[%field_type]"

So for consistency, we should do something like
"widget/formatter plugins provide the config schema for their settings at (widget|formatter).settings.[%plugin_id]"
(instead of entity_view_display.field.[%plugin_id] currently)

Rationale : When I'm writing a widget/formatter, I shouldn't have to know/care about about the concept of "entity view/form display", I'm just writing a widget, and i want to describe its settings.
It's only incidental that EVD/EFD are the primary ConfigEntities enclosing widget/formatter configurations, but they're not the only ones, Views contain configured formatters too...

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 3: 2378055-refactor-entity-view-form-field-schemas.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 3: 2378055-refactor-entity-view-form-field-schemas.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
29.25 KB
1.35 KB

If I use relative dynamic type referencing right, that should fix the fails :) Also noticed two labels missing.

Gábor Hojtsy’s picture

Also I am fine with the widget.settings.* and formatter.settings.* naming, made those changes too. Any other concerns?

Gábor Hojtsy’s picture

Note that for naming things internal to schemas, we should keep in mind that this is the same namespace that is for modules. So if there is a module named formatter or widget, those will share the namespace with these settings. I don't think we have any guidelines for namespacing component schemas where they don't belong to a config key though, so no good guideline to suggest.

yched’s picture

+++ b/core/config/schema/core.entity.schema.yml
@@ -126,6 +127,7 @@ core.entity_form_display.*.*.*:
       label: 'Field display formatters'
       sequence:
         - type: mapping
+          label: 'Field formatter'

s/formatter/widget :-) (twice, actually)

re #17 namespacing: true - event though the current sub-schemas for field.(settings|value).[field_type] are in fact not strictly related to field.module (because of Core's BaseFieldOverrides) - they are not defined in field.schema.yml, for that matter.

Would
field.widget_settings.[widget_type]
field.formatter_settings.[formatter_type]
be better ?

Namespacing is better (with the same "not really field.module actually" restriction), drawback is that it's not really the same "family" than the ones for the field type:
field.field_settings.[field_type]
field.storage_settings.[field_type]
field.value.[field_type]

Or maybe we can use dots to more clearly separate from the above ?
field.widget.settings.[widget_type]
field.formatter.settings.[formatter_type]

Gábor Hojtsy’s picture

Renamed to field.widget.settings and field.formatter.settings so we are good with the namespacing. Also made the textual fix you suggested (and fixed other occurrences of formatter to widget around the same region as appropriate). As far as I see, we are done here and should get this in? :)

yched’s picture

Status: Needs review » Reviewed & tested by the community

Awesome. Thanks @Gábor !

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed dd1350f and pushed to 8.0.x. Thanks!

  • alexpott committed dd1350f on 8.0.x
    Issue #2378055 by Gábor Hojtsy: Reorganise config schema for...

Status: Fixed » Closed (fixed)

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