Fix PHP 8 / Drupal 10 test failures

The last submitted patch, column.patch, failed testing. View results →

Clearly that wasn't the correct fix.

Looking deeper, it seems that the views_aggregator_group_and_compress() function returns something different than all the other aggregator functions. The other functions return an array with a 'column' and 'group' keys, each of which contain a string. While the views_aggregator_group_and_compress() function returns an array that doesn't have either of these keys. I'm not sure exactly what these aggregator functions are supposed to return because the hook that defines the functions isn't documented fully, so there seems to be a lot of latitude in what can be returned. There also seems to be a problem with views_aggregator_range(), which sometimes returns an array in the 'column' key instead of a string.

Because this affects the latest Drupal core version and PHP version, it causes the GitLab CI tests (which by default only run on the current versions) to fail.

Bumping priority.

This patch solved the issue for me locally.

Well like I said in #4, "the views_aggregator_group_and_compress() function returns something different than all the other aggregator functions". Ignoring the output of an aggregator function if it isn't the assumed type, like the patch does, is not the answer. That doesn't fix anything, it just hides the error message. The proper way to fix this going forward is to turn the aggregator functions into plugins so that they can share an interface that ensures they can be called interchangeably and so that code using those functions knows exactly what will be returned by those functions. Most uses of isset(), IMO, are due to bad architecture shaped by the numerous shortcomings of PHP as a language. Sprinkling in more isset() just to make error messages go away doesn't actually make the code better and doesn't actually fix improperly written functions like views_aggregator_group_and_compress(). Now that PHP 8 finally has some 20th Century innovations like return type hinting, we should use those to make the code better.

It might be related. After updating to D10.1, we encounter similar issue:
TypeError: Cannot assign array to property Drupal\views\Plugin\views\field\FieldPluginBase::$last_render of type Drupal\Component\Render\MarkupInterface|string|null in Drupal\views_aggregator\Plugin\views\style\Table->executeAggregationFunctions() (line 540 of modules/contrib/views_aggregator/src/Plugin/views/style/Table.php).

After some debugging, I noticed that the function views_aggregator_tally declares the variable $values = ['column' => []];. However, at the end, the 'column' element will never get a value! In this case, I wonder why the $values has the 'column' element? I also removed $separator_column = empty($separator_column) ? '<br/>' : $separator_column; because it is not being used by the function.

Regarding the PHP version, we are using PHP 8.1, which has no issues with dynamic properties. The dynamic properties are deprecated in PHP 8.2

All of the views_aggregator functions return an array with keys 'column' and/or keys for each group. Each of those array elements is also an array, which contain the values for column aggregation and group aggregation respectively.

This is pretty much undocumented, although views_aggregator_views_aggregation_functions_info() in views_aggregator_functions.inc, and in views_aggregator.api.php explains this somewhat. As well as the comments in src/Plugin/views/style/Table.php

In the Table plugin, the function results are taken out of the 'column' array element or the individual group array elements, as needed.

So because views_aggregator_tally() is declared in views_aggregator_views_aggregation_functions_info() to provide both column and group aggregation, both of those keys are expected in the array returned by that function. So I suspect what you are trying to do is do column aggregation with views_aggregator_tally(), but that function does not seem to have code to do column aggregation. The fix then should also include changing the function declaration in views_aggregator_views_aggregation_functions_info() to reflect this. But it seems views_aggregator_tally() was INTENDED to do column aggregation, so I would like to find out if the code for column integration actually existed in a previous version and got lost, or whether it was never actually implemented.

And this is part of what I was getting at in #8 - without some type of structure like an interface declaring the return types for these functions, there is no mechanism to ensure that they return the right things - that is really the MINIMUM we should be doing. Replacing the return array containing magic undocumented keys with a structured object, or even having different aggregator functions for column and group aggregation is an important thing to do to keep this module viable going forward. And rather than having an _info() hook to discover aggregator functions, these should all be plugins so they don't all have to be declared an loaded on every page in one massive include file but instead can be discovered and loaded from their own individual (and small) files only when and if they are needed.

I think we can fix this issue with views_aggregator_tally() and views_aggregator_group_and_compress() without making these major changes, but we're going to keep running into problems like this until the whole structure of how we define and use these functions is improved and modernized.

Regarding the PHP version, we are using PHP 8.1, which has no issues with dynamic properties. The dynamic properties are deprecated in PHP 8.2

Yes, sorry my initial post is a little inexact. The problem first showed up because core made changes to remove dynamic properties in preparation for PHP 8.2. Core did this by declaring the properties and giving them types. The test failures showed up in this module, and in our PHP 8.1 tests (was not able to test against 8.2 back then), because these core changes were only made in PHP 8 compatible code - for example the union type for last_render is a PHP 8 construct and when core added the property declaration for last_render and used a union type that cause the test failures in our PHP 8.1/D10 tests but not any other tests because D9 core didn't make the same changes and because D10 never supported less than PHP 8.1.

patch #9 works and helps me

tr → changed the visibility of the branch 3344504-fix-php-8 to hidden.

tr → changed the visibility of the branch 2.1.x to hidden.

Over the past few days I've done a lot of work on typing (See 📌 Parameter typing and return type hints Active ) and turned on strict_types in the tests (See 📌 Add declare(strict_types=1) to all tests Active ).

At this point I feel I can start to drill down into the tests and fix the remaining typing issues one by one to get to the root of the two test failures. I will be making a lot of small commits here before this is ready for review.

Oh that worked. Now let me take my "experiment" out and move it to the right place.

I tested patch in my project and got the error

TypeError: Drupal\views_aggregator\Plugin\views\style\Table::setAggregatedGroupValues(): Argument #3 ($group_aggregation_results) must be of type int, string given, called in /var/www/web/modules/contrib/views_aggregator/src/Plugin/views/style/Table.php on line 395 in Drupal\views_aggregator\Plugin\views\style\Table->setAggregatedGroupValues() (line 1153 of /var/www/web/modules/contrib/views_aggregator/src/Plugin/views/style/Table.php).

Needs to add

$group_aggregation_results = (int) $this->options['group_aggregation']['group_aggregation_results'];

for strict types

How did you get that error? I should add a test for that section of code, because it clearly isn't being tested currently.

https://git.drupalcode.org/project/views_aggregator/-/blob/2.1.x/src/Plu...

Last parameter needs to be int, but string is coming

Yes, I can see it is intended to hold an integer value.

But I need to know what you were doing when you got that error so I can replicate that error with a test case.

The schema is wrong. There are at least two other type definitions in the schema that are wrong too, so I would like to write a test case that will check all the options values.

When the schema is corrected, that option will hold an integer value and a cast will not be needed. The solution is to fix the schema.

I just have a views with such settings:

And you see the error when you save the view? Or when you visit the URL of the view? Where do you see the error, in your server logs or your Drupal logs?

I visit the URL of the view. The site has WSOD and I see the error in Drupal dblog

OK, that's what I need to know. Thank you.

I pushed a fix to the MR - can you please try this and see if it solved the problem for you?

You will probably have to re-save the table settings on your view for this change to take effect. Edit the view, click on the "Settings" link next to "Table with aggregation options", then press the "Apply" button, then "Save" the view.

Yes, patch + resave views solved my WSOD

Thanks for looking at that. I'm going to handle the schema fixes in a different issue - 📌 Turn on strict config schema checking Active - because it requires an update function and an update test. I want to get the above fixes in so that the tests are all running before I start messing with new tests.

The update function in that issue will automatically modify existing views, so no one will have to manually save the view with the new settings.

Committed to 2.1.x.

I'm leaving this open to backport to 2.0.x to fix the same failures that occur there.
I'm only going to backport a small portion of this - I'm not going to try to backport all of the typing changes, but just backport the two key sections identified by fixing the typing issues. Namely the return value changes to the tally and range functions.

I tested again. Branch 2.1.x, last commit.

I returned my views and I got WSOD again. Re-save views doesn't help now. Error the same as #21

Do you forget to update schema for 2.1.x branch?

group_aggregation_results is string in schema https://git.drupalcode.org/project/views_aggregator/-/blob/2.1.x/config/...

But function needs int https://git.drupalcode.org/project/views_aggregator/-/blob/2.1.x/src/Plu...

No, I didn't forget. I said in #32 that I was going to do the schema changes in that other issue. I'm writing the test for that now.

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