- 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?
- Merge request !34Issue #3390488 by jordik: Sorting By Incorrect Column On Duplicate Column Names → (Open) created by tr
- Issue was unassigned.
- Status changed to Needs review
about 2 months ago 12:15am 22 February 2025 - 🇺🇸United States tr Cascadia
I put @jordik's patch into a MR for testing. All the tests pass, but that's to be expected because they passed even when this bug *wasn't* fixed.
This still needs a test case, as I mentioned in #5 and #8. The test without the patch should fail, demonstrating the bug, and the test with the patch should work, proving the patch fixes the bug.
If someone wants to write this test, this issue can be fixed right now. Otherwise it's going to have to wait until I can devote some time to doing it myself.