- ๐บ๐ธUnited States tr Cascadia
Well what I'd really like to do is change the return value of ALL those functions, because it's a big problem everywhere. Returning an array where the caller doesn't have any knowledge or guarantee of what can be in the array is just not a sustainable signature. It's trying to overload the return value for a variety of different situations that is the problem, and using an array instead of a typed and documented data structure just compounds the problem.
Right now, the functions return an array that *must* contain a 'column' key and *may* contain other keys of unknown names. There is no way to type-check this to ensure the proper data is being passed around, and no way of really documenting the returned data. When I added strict typing some of the code started throwing errors when the 'column' key was not present - that is $values['column'] was null instead of an array because some of the functions like tally never set the required 'column' key.
Part of the problem is that the tally function is declared to do both group and column aggregation, in
views_aggregator_views_aggregation_functions_info()
, but there is no code to do the column aggregation. I think this was a mistake so maybe if we just change that hook to say that tally can't do column integration then the 'column' key won't be required (or we can tell the calling method to not look for 'column' if the function can't do column integration).So does an empty array need to be used to set $values['column'] (in line 722), given that it is never (supposed to be) updated to any other value within this function as things currently are?
722 looks like it does because that's what we do in all the other aggregator functions. If it fails for tally because one of the groups is named 'column', that means using any other function will have the same problem. But the other functions don't fail because they don't use
$values['column']
to set the value of$values['column']
as is done in tally. They are instead overwriting the group results for the 'column' group with the column results. So just setting$values['column']
to an empty array at the end of the tally function will make the issue go away, but it will overwrite the group key just like all the other functions. That isn't something that should be done IMO. This error has exposed the fact that (I assume) there is a group named 'column' - that's an issue we will have to address everywhere, not just in the tally function.But in the long term, I am going to move all those functions into Plugins where typing and discovery can be done better. I already have this written and in use locally, but I have been busy with other things and it's a fairly low priority for me.
Setting
$values['column']
to and empty array on line 722 is a good workaround to get your site working as it did before, but just be aware that it's hiding a problem in the code that may affect other results if not correct. That's why I want to fix this structural problem, not just hide the error.If you can post your views config that would help. Thanks.
- ๐ฌ๐งUnited Kingdom natts London
So does an empty array need to be set in $values['column'] (in line 722), given that it is never (supposed to be) updated to any other value within this function as things currently are?
- ๐บ๐ธUnited States tr Cascadia
You're right, it's not practical to try to reproduce it under those circumstances, but if you post an export of the view perhaps something in there might catch my eye. I would be looking for how the tally function is used and whether it's being used for group or column aggregation (column is not supported for tally but it shouldn't fail either).
The only way I can see that the code fails is if you have a 'group' named 'column', which I suppose is possible - if that's what is happening that would illustrate once again why this nasty habit in Drupal of passing around keyed arrays instead of proper data structures is still a bad idea.
The reason it works in 2.0 and not 2.1 is because 2.1 is a lot stricter about data types, which reveals cases like this where a wrong data type is being used somewhere. That is beneficial in the long run because it lets us find these problems by generating errors instead of hiding them and hoping they don't affect the results.
- ๐ฌ๐งUnited Kingdom natts London
Just to add that I'm using PHP 8.3.15, on hosting provided by SiteGround.
I have just updated line 722 to be simply:
$values['column'] = implode($seperator_column, array());
And then the view loads fine.
Changing it back as per v2.1.1 breaks it again.
Looks like this line was committed back in October..
Looking at the foreach loop starting line 691, could it be that $values['column'] is getting set to NULL on line 697, because the $group happens to be 'column' in my instance?
- ๐ฌ๐งUnited Kingdom natts London
Thanks for the speedy reply!
I've just replaced the views_aggregator_functions.inc file in my environment, just in case that was that issue, but it wasn't. The bug still exists after clearing caches.
Would you like an export of the Drupal view config? This error has only existed or me since upgrading directly from v2.0.2. The view is an adapted version of the Orders admin page provided by Drupal Commerce, so providing instructions to reproduce would involve a whole installation and configuration of Drupal Commerce, so it's not really practical to do that.
- ๐บ๐ธUnited States tr Cascadia
TypeError: implode(): Argument #2 ($array) must be of type ?array, string given in implode() (line 722 of modules/contrib/views_aggregator/views_aggregator_functions.inc).
The variable
$values['column']
is initialized to an empty array in that function and is never changed, so I don't see how this is possible unless you have an older version of the code.Check your copy of views_aggregator_functions.inc to make sure it's identical to the 2.1.1 version of the code. If it is, I will need instructions for how to reproduce this.
- ๐ต๐ฐPakistan isalmanhaider
It worked!
Thanks @PremSee if you can notify maintainers of the module to approve MR!