Aggregated entity fields cause SchemaIncompleteException

Does this only happen in a test or does it also happen when you try to save the View (which is when the config gets validated in a non-test scenario)?

Unless \Drupal\Core\Config\Development\ConfigSchemaChecker is added as an event subscriber, config schema exceptions will not be raised at runtime.

This is is done for Kernel and Functional tests, which is why schema errors are commonly not found until such a test is written.

I can confirm this bug on Drupal 8.6.10 when running functional tests.

I have a view with aggregation enabled and use MIN and MAX on some fields.
--

Edit: It looks like when using aggregation, the views are being saved with data of multiple schema at the same time when (I assume) they only support a single one.

In my case, fields had a `date` plugin_id and when I enabled MIN or MAX aggregation on them they were "converted" to `numeric`. This caused the options to be saved for both plugins under the same "plugin_id" which is breaking the schema.

I have no knowledge here, but shouldn't these type options be saved separately in an array? Both types are valid and all the options saved seems also required but the way it saves is causing the bug.

Attaching patch with simple solution of adding missing fields into views_field type, which is common for other views fields. It's not clean, but at least has some logic. Another solution is to change whole override functionality, we even have corresponding comment in \Drupal\views\Plugin\ViewsHandlerManager::getHandler(), saying // @todo This is crazy. Find a way to remove the override functionality.
But I'm not sure, whether this type of change is in scope of this issue.

Hi here!

I reviewed the patch and this works well!

However, I have an observation.

The module commerce_reports (https://www.drupal.org/project/commerce_reports) has the view views.view.sales_report.yml that has the fields set_precision, precision, etc.

So I think that the scheme configuration solution that has patch #11 is needed to implement into the commerce_reports module more than views core module.

Thanks for the review. commerce_reports module is only example. Actually you can reproduce this issue by creating test view, showing content of any type, adding field Content: ID, enabling aggregation and setting COUNT type on added field. Then you will see error in configuration, saying missing schema.

I think, adding aggregation fields to schema of views_field type from patch #11 makes sense. This way, schema fix will be applied to any field with aggregation.

I can confirm the patch in #11 works for me, and passed my code review!

Boldly marking as RTBC.

Lets put the solution into the IS and skimming the issue there are steps to reproduce in #14 which should be in the IS as well.

The remaining tasks indicates that a test is to be written yet there are no tests here, adding tag.

Here's a test. Thanks for the patch @ilya.no!

The comments above note the saved config is not in sync with the schema definitions. The straightforward approach seems to be to keep them in sync. This is easily done by changing the plugin_id when the config is saved.

The $all parameter to unpackOptions should not be TRUE by default. This enables the contamination of config with items from two schema definitions. Setting it to FALSE allows views to prune items no longer applicable to the handler.

public function unpackOptions(&$storage, $options, $definition = NULL, $all = TRUE, $check = TRUE)

The test errors were nearly all due to data that is out of sync with the code change. In other words, default module views and test module views need to be updated to reflect the config changes. This includes views in a tar.gz file for testing.

It might make sense to split the patch over new issues:
1) add plugin_id to defineOptions() and schema.yml file
- this requires moving plugin_id in existing default views (regular and test modules)
- this is not directly the topic of this issue but collateral damage of fixing it

2) code change to update plugin_id when grouping applies

Try again.

Add more undefined keys used in code and in tests; not in schema.yml files.

To reiterate, since patch in #24, the test errors are all due to 1) missing schema, 2) code and schema not being in sync, and 3) errors in default views.

Latest patch:
- adds more undefined keys used in code and in tests; not in schema.yml files
- syncs code with schema
- fixes default views

More of the same.

NW for issue summary update

@andypost Have you read the related issue 3258664? It has the issue summary update. This issue is RTBC in reality.

@solotandem yes, but "no idea" as proposed will not work for commiters

This is only a rebase for now.

@vacho your patch contains lots of unrelated changes, please clean-up it and reupload

Yea @andypost, thanks to this catch.

Now yes, a good rebase.

Also looks right

/v/w/h/contributedrupal10 (10.1.x|●33…) $ drush cr
 [success] Cache rebuild complete.
/v/w/h/contributedrupal10 (10.1.x|●33…) $ drush config:inspect --only-error --detail
 ----- -------- 
  Key   Status  
 ----- -------- 

issue summary still needs update

i have fixed cs error.Please review it

2 tests still broken and not clear how it will work for existing sites/views

Updating patch for 10.1.x and Fixing Custom comand failed #47

After review, the last patch needs these works:

- Removes all code comments. Also, explain what the task remains to fix on these.
- Is not clear to me: why changing the order of the plugin_id: field to the first position fixes something.

As I see this looks like the path to fix this issue:
- After saving the view (with aggregation and...) at a time to export the views configuration, this should reflect the plugin_id that views use internally, also the schema should cover this scenario when some item turns as aggregation.

Also one note: Currently, with the last code of Drupal10 I don't get the issue creating a view as #14 explains, how to reproduce. Can some check if still happens and comment on the exact steps to reproduce it?

Related to that I can't reproduce the issue anymore.

I follow these steps:
- Create a view for filter fields for Basic page CT
- Let only a field for Content ID
- Add Aggregation - Count for this field.

This is the git diff for view configuration and these don't throw a config inspection issue.
Not that any field changes of the type.

this is the view configuration by GUI:

maybe the issue doesn't happen anymore?

This is only a palliative solution. I found this issue in a doesn't upgraded to D10 project.

So this hack propose: If the view field change to numeric, and the schema doesn't chain, the view field can contain these keys(for numeric) also and this can avoid the issue.

These case for schemes with fields that not always be used, was used before on Drupal. So it is no a crazy solution.

Good point with schema but it means we have no test coverage for this handler, IS also needs update

I've tested on D10 site and haven't reproduced issue neither. I think, that patch from comment #51 is enough for sites, using D9, while issue itself may be closed.

Still needs tests and an issue summary.

If not an issue for 10 then that should be documented in the code and version of the ticket changed.

Attaching patch with tests, which are taken from #20 comment. I've also updated issue summary with 2 possible solutions.

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

While checking failed tests for my patch, I've discovered following points:
1) My solution doesn't work for latest versions of Drupal, because it adds more mess into config schema definition and it affects views more heavy, than I've expected, so I've focused on another solution of that issue.
2) I've put a bit wrong description on `Steps to reproduce` section. If we create field and then enable aggregation, than issue is not reproduced. But if we create view, enable aggregation, add field and set its aggregation to COUNT, than issue can be reproduced with latest version of Drupal. So we first need to enable aggregation and only then add field and set aggregation for it.

I've fixed `Steps to reproduce` section and also removed my solution from `Proposed resolution` section. I'm adding new patch, which is rebase of the patch from 33 comment. I couldn't generate interdiff, but basically it's rebase for D11 with couple of fixes. I've also added hook_post_update. I haven't run all tests, but the ones, which were failing are now good, at least locally.

I've created MR with changes from latest patch for further developing.

Thank you 🙏 Is there a way to prevent reordering in yml files?

As I understand no, because currently different mappings in schema contain plugin_id in their own place. In this issue we remove plugin_id from all these mappings and add it to the `views_handler` type at the first place.

Basically we don't need to reorder it for existing views, but we need to update default views from core modules, because otherwise tests fail. It happens, because test compares default config with actual and fails, as they are not the same.

I've pushed phpcs fix into MR, not sure, that I need to update patch for it.

Appears to have test failures. Also believe we will need a simple test for the post_update hook.