Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Steps to reproduce
- Add a taxonomy field to user
- Add a user view
- Add the user: field_tags relationships to the user
- You should be able to add taxonomy term: name and other fields, but it doesn't work
Sadly we forgot to change the code of entity_reference.views.inc in #2429447: Use data table as views base table, if available.
so the relationships introduced
Proposed resolution
Add them, as well as test coverage
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#30 | entity_reference_joins-2451789-30.patch | 47.9 KB | jibran |
#30 | interdiff.txt | 7.35 KB | jibran |
Comments
Comment #1
dawehnerComment #2
dawehnerComment #3
olli CreditAttribution: olli commentedHere's a start.
Comment #4
olli CreditAttribution: olli commentedAdded test for entity reference.
Comment #6
Anonymous (not verified) CreditAttribution: Anonymous at Druid commentedAtleast on views entity references work again with this patch.
Comment #7
jhedstromRemoving the 'needs tests' tag. The fix looks solid to me, and adds test coverage.
Comment #8
lauriiiLooks good to me too
Comment #9
alexpottAre we not missing test coverage wrt to this change and the original bug report?
Comment #10
olli CreditAttribution: olli commentedThis is entity_type elsewhere. Added join_extra.
Made the same change for file and image.
Still needs more tests.
Comment #13
dawehnerI like the better help text.
Its good to have these 1 line summary documentation, this makes it really easy to scan the code.
Comment #15
lauriiiTaxonomy references have been removed #1847596: Remove Taxonomy term reference field in favor of Entity reference
Comment #17
olli CreditAttribution: olli commentedComment #19
dawehner@olli
This is not really what I meant.
You added new tests which ensure that specific views data is generated ... which is not a bad idea, but we still haven't a proper test that an entity reference query work as expected.
Comment #20
dawehnerNeeds work based upon my last feedback, sorry :)
Comment #21
jibranI don't think we need this.
Comment #22
dawehnerAre you sure about this? Other fields do a similar kind of join:
Comment #23
jibranYeah you are right please ignore me.
Comment #24
jibranHaving a look at it.
Comment #25
jibranI hope this address @dawehner concerns.
Comment #27
nuwe CreditAttribution: nuwe commentedComment #28
dawehnerThank you for working on it, great work!
@nuwe
Thank you for adding the tag.
This just fixes some bug on an API level, screenshots would not really add an value, IMHO :)
But sure we should post more screenshots of stopped debuggers.
You could have tried to post an interdiff just containing the new test coverage ... :)
But sure +1 for renaming variables to be more sane and moving fields around in order to make it easier to read!
This is a bit odd ... why do we get the index but then just use it to access
$view->result[$index}
?Can't we just use foreach ($view->result as $row) and then use $row?
These tests are looking great! Exactly what I thought we should have test coverage for
You can skip the destroying if you reload the view anyway, just a tip.
Comment #29
olli CreditAttribution: olli commentedI think those exceptions are caused by entity_test_mul_default_value entity type having the same base/data table as entity_test_mul. Test installs only entity_test_mul but views tries to load entity_test_mul_default_value... Should we change entity_test_mul_default_value table names?
Comment #30
jibranThanks for the review @dawehner fixed everything.
@olli you are correct. I have changed the table name of entity_test_mul_default_value now the tests are passing but I'd not be surprised seeing some other tests failing in core.
Comment #31
dawehnerYeah, this is clearly a bug and its just luck that things work at the moment.
Comment #32
jibranIs it a views data bug?
Comment #33
alexpottThis issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed b58ab8c and pushed to 8.0.x. Thanks!
Comment #36
jweowu CreditAttribution: jweowu at Catalyst IT commentedI wonder if someone from this issue can assist with this?
https://www.drupal.org/project/drupal/issues/571548#comment-14590720