Not all shipped configuration passes validation even with all modules enabled

Discussed the display.default.display_options.filters.langcode.value missing schema one with @alexpott. We figured the language filter (Drupal\language\Plugin\views\filter\LanguageFilter) is not actually dependent on language module, so can move to views with its schema, so it would be available generally.

Also discussed the other two Views ones with @alexpott and as per him, discussion in Austin resulted in that optional dependencies like that in config entities will not be possible. Content translation will need to provide an alternate view on its own for the front page. So if we consider that translation link feature important enough to warrant a new view, we should make a copy of the front page view and the user admin view in content translation and provide it from there. Then remove these fields from the standard views. BTW #2161845: Node views (front page, admin) do not use the proper language filter proposes to make the front page view even more tailored for language.

These changes will allow to provide a test for all the things. Again @alexpott suggests we put the test in ConfigImportAllTest since it does all the prepwork.

So those should provide the way for the trickier parts of the issue. Hopefully the other ones are not optional dependencies :)

Looks good. Are you up to continuing with the views issues as per above? :) That would allow to add a test as well so all standard profile config is tested. Needs work to add the remaining things, not because I found anything wrong :)

Why not fix all 3 here? Then we can add an overall test on the standard profile and ensure it will not fail again at least for shipped standard config. I consider that way better compared to keep doing one-off fixes and hope nobody introduces more bugs, while they in fact do...

So I think the scope of this issue is to make all standard configuration pass validation, period. I think the attached test would theoretically prove that. I did not test it though :D Retitled to make it clearer. Adding the test is the only way to make this not happen again and again.

How did I not recognise that PHP error :D

Woah lots more fails.

- entity.form_display.***:content.path.settings: Missing schema.
- field.field.**:settings.translation_sync: Missing schema.
- field.instance.**:settings.translation_sync: Missing schema.
- migrate.migration.*: BAZILLIONS of top level items across lots of files: Missing schema.

Most bizarre that it DID NOT fail on the shortcut or views that we expected. Heh.

Oh, there is a pattern to the migrate fails. These are not in the migration files shipped but I guess they get added when imported. Top level keys: row, idMap, sourceIds, destinationIds, highwaterProperty, systemOfRecord, sourceRowStatus, trackLastImported. All of these are missing schema for all of the migrate files, so it fails 8 times for all migrate files. No wonder the hundreds of fails.

So this test actually enabled all the modules so we are testing against a full site. So the optional dependencies will not come up here and we can move that to a different issue :) At least this will ensure if ALL modules are enabled then the schemas are correct for all shipped config, which is very valuable in itself. We should open an issue for the views problems, but the rest should still be fixed there to make the test green :)

Fixing all the migrate fails. The defaults for file field display settings were not the right type. The schema definition on images was also not the right type, checked with the form code that it is a checkbox on images as well. The rest not fixed yet.

Most of the rest of the new fails were due to the workaround for translation sync, since it was not picking up the field specific elements anymore. I looked into introducing a field_settings base type but there are no common settings between all the fields whatsoever. translation_sync would be (also on field instances) but that is better dealt with in a test exception I think.

This I think may still fail on entity display issues (additionally to all the translation_sync fails).

The shortcut schema includes the list of shortcuts element which is not there anymore, the shortcuts are now content entities and not stored in the config entity. Fixed that.

The entity display schema for the path field was defined for the entity *view* display not for the form display. There were never fails for the view display, but there are for the forms. The schema looks correct for the form although I'm not 100% positive on how to verify, the form has these elements. So I think vijaycs85 just misnamed the element in the schema.

This should now only fail on translation_sync.

All right, this one should fix all remaining fails. I opted to have test exceptions for the translation_sync setting only. This ensures that all other parts of all other files are tested. I think it would be really valuable to get this one in with the workaround and remove later.

@vijaycs85 pointed out that we should not remove the links element from the shortcut set because a null is still written out somehow for that element, so we need to figure out why. The reason ironically is the schema itself. Now the schema is used to persist config entities, and since the links element was there, even though shortcut sets don't have that anymore, the process looks up that property and takes a null value, since it is not there. That results in a null export due to the schema. So we need to remove the schema (which results in no links element exported).

Updated the issue summary. Opened #2301045: Standard profile has views which include elements dependent on uninstalled modules, not valid in config for the views issues. Once this is fixed, only that should remain for standard profile. Remaining tasks now accurate, with the two questions I think we need to figure out. Assigning this to alexpott as CMI maintainer for those two questions.

@andypost points out path form displays don't have settings (see image by @andpost). Righto.

- Fixed the path schema so it again contains a settings key (but no element definition since no elements are allowed).
- Added more specifics with config names to the workaround in the test on translation_sync
- Add comment on test to ConfigImportAllTest
- Remove the unnecessary elements from the migrate schema, we'll see :D

So looked into the common properties to ER, taxonomy, files and images.

Issues found with entity reference:
- target_bundle was missing from schema, but in settings
- the handler_settings in instance settings is only on this type, altered in, not on the other types, so we cannot inherit from the instance settings; we could add a base type but for the handler key only that sounds a bit superfluous

Issues found with taxonomy:
- the instance settings should be a mapping not a sequence, has a handler key

Issues found with file:
- has most instance settings common with images but the display_field is only here (unset in image), so a new type was needed

Issues found with image:
- instance settings should depend on base type from file

I think this covers the data properly now :) Looking forward to test run.

@andypost: well, currently it is a string. #2064191: Fix the 'target_bundle' field setting - 'target_bundles' are not validated when entity_reference is enabled says it should be an array, but there is no such array on the entity reference settings top level now is there? There *is* a target_bundles on the handler settings, that is already covered by the schema as well that I see. So I think you are pointing out possible future changes and not issues with the patch?

All right, in IRC, @anypost pointed to this:

class ConfigurableEntityReferenceItem extends EntityReferenceItem implements AllowedValuesInterface {

  public static function defaultSettings() {
    $settings = parent::defaultSettings();
    // The target bundle is handled by the 'target_bundles' property in the
    // 'handler_settings' instance setting.
    unset($settings['target_bundle']);
    return $settings;
  }

That's not schema compatible at all. Schema cannot be written for something and then depend on the class implementing it. Unless the data would somehow indicate which one it is, but here it tries to disguise as the other one. Lol.

function entity_reference_field_info_alter(&$info) {
  // ...  
  $info['entity_reference']['class'] = '\Drupal\entity_reference\ConfigurableEntityReferenceItem';
  // ...
}

I don't think we have any other way right now but to keep that property on the schema because we cannot know in any way which implementation is running or which implementation a config file belongs to for that matter.

Also that does not result in test fails here in any way and I would avoid turning this into a "fix everything related" issue :) The added tests will be very valuable and should keep people in check at least for more obvious mistakes.

So all the things I found in this exploration:
- entity_reference field type is in fact defined by code, but no schema is provided by core
- the configurable entity reference alters in implementation for entity_reference AND defines the schema
- since they use the same type name, there is impossible to provide distinct schemas for them
=> so we need to keep not providing a schema for the base type

- the configurable entity reference removes the target_bundle key
=> so it should not be on its schema

- file and taxonomy module are inheriting from the core entity_reference implementation, they do not depend on the entity reference *module*
=> they will not have access to inherit from their schema, so we need to start their schemas fresh (we can keep the file -> image inheritance)

In short very flexible magic things that the schema is not fully ready to follow.

So taxonomy should have had target_type as well. Oh well. We can introduce a base type for the combo of target bundle and type then BUT not use it with the actual configurable entity reference schema because that does not have the target bundle. Yeah.

Postponed #2301045: Standard profile has views which include elements dependent on uninstalled modules, not valid in config on this one, since its test would result in overlapping problems with this one. We can write a test for that once these issues are fixed. Let's get it in? :)

Works for me. Moved there.

Yay for tests! Yay!

Yay, thanks!