- Issue created by @mitchmccoy
- Assigned to jordik
- 🇺🇸United States tr Cascadia
That piece of code was changed in #3059245: Rewrite main module functions to work with raw values → by @JordiK, so I'm assigning the issue to him to look at because I don't know exactly what the intent was in that change.
I don't think your workaround is the correct way to fix it though, because if the entity_id array element is not being accessed then we don't need to check to see if it's set anymore. But why was it being used in the first place? @JordiK should know ...
- 🇺🇸United States tr Cascadia
Upon re-reading my response in #3, I see it doesn't make sense, even to me. I think I must have posted that response in the wrong issue, because I talk about something that is not relevant to this problem. And specifically this piece of code was *not* altered or added by #3059245: Rewrite main module functions to work with raw values → like I said.
Sorry for the mistake.
I think we need a test case to demonstrate the problem, which will also prove the proposed solution. That's going to be the hold-up in getting this fixed, but this module is not very maintainable without a good set of test cases and we don't have that yet. So when something like this is identified as a bug, it's extremely important to me that we write a test case to prove that the fixed code does things correctly.
- 🇺🇸United States tr Cascadia
We already have tests for sorting, in ViewsAggregatorStyleTableTest. This uses a test view with id va_test_style_table which is provided by the views_aggregator_test_config test module.
I think we can add a test case for this issue with minimal effort, by modifying that test table so that it has the two columns/same name situation described in the OP. Then the already existing sorting tests should fail without the patch from #6?