When creating a view with one or more table relationships, it's possible that you have duplicate column names.
i.e. table1 > name
table2 > name
When you try to make both fields sortable, clicking on Name (table 2) column, actually sorts the table by Name (table 1).
The problem appears to be in:
/modules/contrib/views_aggregator/src/Plugin/views/style/Table::compareResultRows line 1410.
1. Create a new view that against a table.
2. Add a relationship to a 2nd table that has a duplicate column name. i.e Table 1 > UserID, Table 2 > UserID.
3. Make both columns sortable in the Table Aggregator Settings.
4. View the report and sort on each of the columns.
5. They'll both sort based on the first column, never the 2nd.
I created a workaround by replacing on line 1410: $field_handler->options['entity_field'] with $this->active.
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | Sorting_by_Incorrect_column_on_duplicate_column_names-3390488-6.patch | 672 bytes | jordik |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
mitchmccoy commentedComment #3
tr commentedThat 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 ...
Comment #4
tr commentedComment #5
tr commentedUpon 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.
Comment #6
jordik commentedGood catch @mitchmccoy! Please check the patch.
Comment #7
jordik commentedComment #8
tr commentedWe 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?
Comment #10
tr commentedI 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.
Comment #11
tr commentedStill needs tests.