Aggregated entity fields cause SchemaIncompleteException

Created on 31 May 2018, about 6 years ago
Updated 28 January 2024, 5 months ago

Problem/Motivation

While working on #2976147: Create a sales report supporting daily, weekly, monthly, and yearly breakdowns. β†’ , my tests failed due to configuration schema validation.

Schema errors for views.view.sales_report with the following errors: views.view.sales_report:display.default.display_options.fields.report_id.set_precision missing schema

The report_id is the entity identifier base field. The View has aggregation enabled, and the report_id is set to COUNT.

In Views, the handler is swapped to numeric as an override when the display is built. This is causing a SchemaIncompleteException exception to be thrown.

This following code is what changes the handler and causes the schema error

\Drupal\views\Plugin\views\display\DisplayPluginBase::getHandlers

        // If aggregation is on, the group type might override the actual
        // handler that is in use. This piece of code checks that and,
        // if necessary, sets the override handler.
        $override = NULL;
        if ($this->useGroupBy() && !empty($info['group_type'])) {
          if (empty($this->view->query)) {
            $this->view->initQuery();
          }
          $aggregate = $this->view->query->getAggregationInfo();
          if (!empty($aggregate[$info['group_type']]['handler'][$type])) {
            $override = $aggregate[$info['group_type']]['handler'][$type];
          }
        }

Steps to reproduce

Create test view, showing content of any type. Enable aggregation. Add field 'Content: ID' and set COUNT type of aggregation on added field.

Proposed resolution

Update logic of manipulating on 'plugin_id' option and change corresponding code, so plugin_id reflects actual state of the field.

Remaining tasks

Review patch/MR, give feedback, rework, accept.

πŸ› Bug report
Status

Needs work

Version

11.0 πŸ”₯

Component
ViewsΒ  β†’

Last updated less than a minute ago

Created by

πŸ‡ΊπŸ‡ΈUnited States mglaman WI, USA

Live updates comments and jobs are added and updated live.
  • Needs issue summary update

    Issue summaries save everyone time if they are kept up-to-date. See Update issue summary task instructions.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡«πŸ‡·France andypost

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

  • Status changed to Needs review over 1 year ago
  • πŸ‡·πŸ‡ΊRussia ilya.no

    I've tested on D10 site and haven't reproduced issue neither. I think, that patch from comment #51 πŸ› Aggregated entity fields cause SchemaIncompleteException Needs review is enough for sites, using D9, while issue itself may be closed.

  • Status changed to Needs work over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    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.

  • Status changed to Needs review 7 months ago
  • πŸ‡·πŸ‡ΊRussia ilya.no

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

  • Open in Jenkins β†’ Open on Drupal.org β†’
    Environment: PHP 8.1 & MariaDB 10.3.22
    last update 7 months ago
    30,446 pass, 8 fail
  • Status changed to Needs work 7 months ago
  • 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.)

  • Status changed to Needs review 5 months ago
  • πŸ‡·πŸ‡ΊRussia ilya.no

    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.

  • Merge request !6331Fix schema issue on aggregated fields β†’ (Open) created by ilya.no
  • last update 5 months ago
    Custom Commands Failed
  • πŸ‡·πŸ‡ΊRussia ilya.no

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

  • Pipeline finished with Failed
    5 months ago
    #82751
  • πŸ‡«πŸ‡·France andypost

    Thank you πŸ™ Is there a way to prevent reordering in yml files?

  • πŸ‡·πŸ‡ΊRussia ilya.no

    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.

  • Pipeline finished with Failed
    5 months ago
    #82755
  • Status changed to Needs work 5 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

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

  • Pipeline finished with Failed
    28 days ago
    Total: 692s
    #187242
Production build 0.69.0 2024