Problem/Motivation
Lots of errors in #2183983: Find hidden configuration schema issues are related to entity form display and view display errors. Some problems identified:
- missing some core widget types such as email_default, string_textarea
- the base schema does not include settings, so each type needs to include a settings key (looooooots of cruft)
- that cruft is not even honest, those types have no settings, so should have no settings items (an empty sequence fits that)
- by including the settings key in the base schema, we make it possible to not need to define testing types such as field_plugins_test_text_widget
Proposed resolution
Add settings to base schema as empty list.
Remove superfluous cruft.
Add schema for email_default and string_textarea
Remaining tasks
Review. Commit.
User interface changes
None.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#69 | 2368349-69.patch | 37.08 KB | Gábor Hojtsy |
#69 | interdiff.txt | 2.92 KB | Gábor Hojtsy |
#61 | interdiff.txt | 2.44 KB | Gábor Hojtsy |
#61 | 2368349-61.patch | 34.16 KB | Gábor Hojtsy |
#59 | interdiff.txt | 1.88 KB | Gábor Hojtsy |
Comments
Comment #1
Gábor HojtsyComment #2
Gábor HojtsyI don't have a great idea how to test these ones. The fails in #2183983: Find hidden configuration schema issues are all over the place, the fixes are also all over the place. They are *very* straightforward though.
Comment #3
Gábor HojtsyComment #5
BerdirOn the last comment, most field types have two tests:
- a kernel test, that just creates data and saves it. (e.g. NumberItemTest, and similar)
- a web test, that also tests widgets and formatters. (e.g. NumberFieldTest and similar)
We could try add schema validation to those. Having a single test that creates 15 different fields and tests them all seems crazy, as we already do that.
Maybe we can have a common base class for those tests, and have a tearDown() that validates this, if possible? We do something similar for php errors, so why not schema validation as well?
Comment #6
Gábor HojtsyIn fact this removal of the arbitrary sequence of settings exposed lots of fails :) So looks like plenty of things to fix here. Checked settings for those field types.
Comment #8
Gábor HojtsyI think the fails we already have here showcase that the existing schema validation works very well if instead of an obscure sequence of strings, we define them with their actual keys. So in my view the existing fails may be well enough of a proof, no?
Comment #9
Gábor HojtsyThe tests even found this :)
Comment #10
vijaycs85wow, that's very nice cleanup. Thanks @Gábor Hojtsy :) +1 to add more tests.
Comment #11
Gábor HojtsyI would argue this does not need more tests. The reason the tests were not failing is that the form modes and view modes had this section for settings that slurped up all subkeys whetever they were:
This is a classic old example of a "I have no settings" definition from before we made the sequence key of the sequence type optional (and resolve to undefined types for the children). Once we introduce a settings key globally for the base type of the entity view and form elements, any key under them that was not defined starts to throw errors big time as shown above. This was already demonstrated in #6 and #1 for several types but not the string_textarea and email_default types because those were already added. Here is a test targeted patch that does not include those fixes or any of the other fixes either.
It just removes the "anonymous sequences" and adds empty sequence definitions, which thus fails with the existing tests for each entity view or form field section that does have specific keys defined (that is should not have empty settings).
This should fail for all the items fixed in #9:
- entity_form_display.field.string_textarea
- entity_form_display.field.email_default
- entity_form_display.field.boolean_checkbox (promoted, sticky)
- entity_view_display.field.comment_default
- entity_view_display.field.image
Given we already have tests for field types, the definition of their entity view/form settings as an empty sequence is enough to fail on all the errors. Not defining their settings key or defining it as an anonymous sequence of strings ate up all the settings and obscured the fails. A sequence that was defined to not have items fails very well already if it has items for some reason.
Going forward we should consciously remove other instances of anonymous sequences which were used to define sequences we want to be empty, and use sequences that are defined to be empty, so that we get all the fails if they contain elements.
Comment #13
Gábor HojtsyWell, right that did not fail for string_textarea and email_default. Thats interesting because EmailFieldTest creates email_default fields. I could not find a test that creates string_textarea fields. Which should be that one? @Berdir?
Comment #14
Gábor HojtsyAll right. The existing fails are due to those fields appearing in default config where they are already tested. I added that base test class with a tearDown() and added to a few field tests I found. As said, I did not find the string field test. There are also tests that inherit from other base classes, such as the options field test :/
This still does not include the actual fixes, so we can see if there are more fails or not. The email one should finally fail. I don't know what else may start failing in the other displays.
Comment #16
Gábor HojtsyWow, Uncaught exception 'LogicException' with message 'Missing phpDoc on Drupal\field\Tests\FieldItemTestBase.' :)
Comment #17
BerdirNot correct, a base class must be abstract only actual test classes should have a @group.
Comment #19
Gábor Hojtsy@Berdir: indeed. It still stands that the string field type does not seem to have tests. We need to introduce one looks like.
Comment #20
Gábor HojtsyCreated a very quick StringFieldTest by copying the relevant part of TextFieldTest to this. I made StringFieldTest extend from FieldItemTestBase, so it gets all the schema checking. Then TectFieldTest lost its common parts with StringFieldTest (testing the no-formatting widgets and most of the setup method). I think this test structure shows the common parts very well, right?
This now fails properly for the string type locally too. So this will be the "test only patch". Note that it NEEDS to include the removal of the eat-all-anonymous-sequences for it to fail properly otherwise those would eat the named settings and not fail. Without those changes, this patch would only fail for the string type and email type that was not explicitly defined before (and therefore had no settings key in the fallback definition).
Comment #21
Gábor HojtsyAnd now including all the fixes again that were in #9.
Comment #25
Gábor HojtsyYay new fails in datetime and responsive images. Yay for tests :) Now onto fixing them.
Comment #26
Gábor HojtsySo those fails are not schema fails per say, they are incorrect data in the test. The datetime test modifies the display type without removing the setting that is not valid for the new field type (which cannot have settings). The responsive image fail is an extra key in the display data that is not supposed to be there.
I think this will finally be green and is complete for the scope of this issue.
Comment #27
BerdirYeah, we only added those tests for existing field types when porting them to the new API but not for those types that were added initially.
WebTestBase use is not removed here.
I think FieldTypeTestBase would be a better name for this...
The scope of this issue is specific to form/view displays, but what about extending this to also cover field and field storage configuration? That will us give more test coverage for those cases, which seems like a good idea. And might also uncover more schema issues.
Can't comment on the schema changes, but the fixes look good to me.
Comment #28
Gábor Hojtsy@Berdir: I'll do 1 and 2 once I get a chance. Agreed. On 3, I am happy to open a followup, the schema bug finder found plenty issues in field schemas AFAIR so that is a logical next step. Happy to work on that too :)
Comment #29
Gábor HojtsyFixed those two and found 2 more unused WebTestBase uses. AFAIS this should be complete and done :) I'll open that followup next and postpone on this one.
Comment #30
Gábor HojtsyNow opened #2370305: Refactor field type configuration schemas for DX, easier to find errors and postponed on this issue.
Looks good to go? :)
Comment #31
vijaycs85Looks good. Let's get this in.
Comment #32
alexpottThis would seem very susceptible to losing test coverage over time. For example, if a test is changed to delete the displays as part of the test then suddenly something that was tested now is not. And it would be hard to spot.
Comment #33
Gábor Hojtsy@alexpott: do you mean we should create tests than that copy-paste essentially all field type initialisation code or somehow tries doing field config with all defaults? Berdir argued above that would be lots of duplicates.
Comment #34
alexpottI've got an idea - working on a patch.
Comment #35
Gábor HojtsyFYI the followup at #2370305: Refactor field type configuration schemas for DX, easier to find errors already has a complete patch and is green :) Needs review. (The test part is dependent on this test though).
Comment #36
Gábor HojtsyAlso even if new methods of testing are introduced, it would be important to keep the string field test (and text field test inheriting from it) from this patch because that is plugging a hole pretty much in field testing in general.
Comment #37
xjmPromoting to critical since #2183983: Find hidden configuration schema issues was postponed on this issue. Also I think this is techically an entity system bug.
Comment #38
alexpottre #34 the idea grew to become a separate issue since it will easier to review as a standalone patch see #2371843: Add event listener to check schema on config save.
Comment #39
jibranAdded the related issue.
Comment #40
yched CreditAttribution: yched commentedNote than no accurate schema for EVD and EFD can be written until #1875974: Abstract 'component type' specific code out of EntityDisplay.
The Displays store several types of components with sub-entries that depend on the type of the component (field, extra field currently + contrib component types). Yet the current code doesn't store the component type.
Comment #41
alexpottCHanged the patch around to use the schema checking exception introduced by #2371843: Add event listener to check schema on config save
The test only patch is the patch without any schema fixes.
Comment #44
alexpottThis should fix up the schema fails.
Comment #45
effulgentsia CreditAttribution: effulgentsia commentedThis was RTBC in #31. The interdiffs since then look great, so back to RTBC.
Just a couple questions, but both can be followup material.
Should we have something like a
none
orempty
type to enforce emptiness instead of allowing it to be anything?Should we make the same change (i.e., add an
unset()
) for'required'
as well? For that matter, should we change the signature of createImageField() to pass in $description and $required separately from $field_settings?Comment #46
Gábor Hojtsy@effulgentsia:
1. We can only have empty lists :) Keys that always need to be empty valued should not be saved into config, right? This case, the ignore type is used for an empty list as well. The correct type to use for that is sequence (without any further definition for items). In that case if the list has any items, config identifies them with an 'undefined' type and will throw errors. Defining it as 'ignore' means IF it has items, config will not throw errors, even though that should not be valid. Ignore removes the item from validation, while defining it as an empty sequence keeps it in the validation proper. So the correct type to use would be sequence. This should be fixed in this issue since its introduced in this issue.
2. I don't have a strong feeling on that one and agreed can be a followup.
Comment #47
alexpottSuch a minor change that I'm setting this to rtbc.
Comment #48
Gábor HojtsyAgreed on RTBC.
Comment #49
Gábor HojtsyWait, I looked at the issues found in #2370305: Refactor field type configuration schemas for DX, easier to find errors because I realized those changes should not be necessary once this lands, since that would have expanded the schema test coverage, but then this did a lot more so.
BUT the fixes in that patch does not align with fixes here.
1. The image field description related fix is different there but is better here.
2. The decimal separator on decimal fields fix however is not correct here. DecimalItem does not have any field settings for decimal separators. It is incorrectly passed in in the test and just ends up in the settings accidentally. The correct fix is not to set nonexistent field settings, not to document that they exist :)
Closing #2370305: Refactor field type configuration schemas for DX, easier to find errors down as duplicate.
Comment #50
alexpottNice catch.
Comment #52
Gábor HojtsySome newly updated core config used incorrect data types for display_label on checkboxes and pager_id on comments. These were undefined settings keys before this patch and therefore saved as string prior to this patch. Now that they are defined, validating them from default config will fail because they need to be bool and integer respectively.
Comment #53
alexpottYep - yay for
Drupal\config\Tests\DefaultConfigTest
Comment #54
yched CreditAttribution: yched commentedWow - it's really wrong to have "Database storage size" as a *field* setting rather than a *storage* setting - but that's indeed what IntegerItem does.
Opened #2376689: IntegerItem 'size' setting should be a storage setting for this.
Comment #55
yched CreditAttribution: yched commentedSorry, hold on a bit plz. Reviewing, but cannot post atm.
Comment #56
xjmComment #57
yched CreditAttribution: yched commentedSo I was bummed by the organization of the schema info: it 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.
Then I noticed this is already the case in HEAD and is not introduced here - however, getting this straight later will be an API change (changes the way widgets/formatters provide schema info). So - should we fix this as part of the critical here ?
Comment #58
yched CreditAttribution: yched commentedOther than that :
OK, so saving a Display with a component that has widget/formatter settings unknown to the widget/formatter contradicts the schema.
Then it seems EntityDisplay::setComponent() should only accept settings that are correct for the formatter / widget - since saving them would be invalid ?
Could be done by changing this in (Widget|Formatter)PluginManager::prepareConfiguration() :
Agreed with @effulgentsia that the createImageField() method is sick indeed, passing 'description' next to the actual field settings is really wrong.
But yeah, fine with a followup - opened #2376899: ImageFieldTestBase::createImageField() takes a description in field settings errorneously
Comment #59
Gábor Hojtsy@yched:
Re #57: This issue is set out to fix the schema errors not to refactor them completely for displays. If we are to do that then the actual changes will be impossible to review. It the same problem in #2370305: Refactor field type configuration schemas for DX, easier to find errors that the refactoring makes it easy to find errors but the more we fix in the issue the harder it is to review due to the whole rename process. As a sub-issue of #2183983: Find hidden configuration schema issues this issue is supposed to fix errors so we can increase our application of strict schema adherence. Fixing existing errors and adding test coverage is what gets us there and then we can refactor. Now in many cases we don't really know if the schemas are good or not so we need to fix and pass tests first.
Re #58/1: Implemented. Looking forward to any possible new fails for things we may not have recognized :)
Re #58/2: Thanks for opening the followup.
Comment #61
Gábor Hojtsy1. The MigrateDrupal6Test has been asserting setting for integer fields that are not there. Fixed test to be more focused.
2. DisplayApiTest uses a field_test_multiple field type with a made-up 'alter' key to then test altering support in field_test_entity_display_build_alter. By documenting this key on this field type, it will pass along properly. This is a one-off made-up key, not a generic feature, only for testing.
These should fix the test fails with the new stricter code. Good to go now? :)
Comment #63
yched CreditAttribution: yched commented@Gábor : makes sense, thanks!
RTBC +1 when green again.
Comment #68
Gábor HojtsySo we need to figure out why my fix works in the context of the whole MigrateDrupal6Test suite but not when running standalone in MigrateFieldFormatterSettingsTest and then we would have cracked it. I have no idea yet, the number/integer field is core :/
Comment #69
Gábor HojtsyOne way to solve this is to assert migration results with settings only relevant for the given field type. Current tests share migration target test config between decimal and integer fields and attempt to set the same keys for both in migrations, which is not good :)
Would really love if we can stop extending the scope of this issue even further now.
Comment #70
alexpott@Gábor Hojtsy yep I agree - let's get this done - scope has crept far enough.
fyi: when run as part of
MigrateDrupal6Test
theMigrateFieldFormatterSettingsTest::setUp()
does not run - this the reason for the difference.Comment #72
catchCommitted/pushed to 8.0.x, thanks! Agreed on keeping this moving to unblock other work.
@yched can you open the follow-up for the schema re-organisation?
Comment #73
Gábor HojtsyYay! Posted two versions for the issue at #2376899: ImageFieldTestBase::createImageField() takes a description in field settings errorneously which was our first followup.
Also sent #2370305: Refactor field type configuration schemas for DX, easier to find errors for a retest, which is our next big schema issue to hunt down :)
Comment #74
yched CreditAttribution: yched commented@catch: opened #2378055: Reorganise config schema for entity_form_display / entity_view_display.
Since that will be an API break, committers green-light is welcome over there :-)