As discussed in #1893772: Move entity-type specific storage logic into entity classes, loading is the wrong place to check for term access. We're not doing this for any other entity.

CommentFileSizeAuthor
#9 drupal-1919016-9.patch1.75 KBdawehner
#9 interdiff.txt940 bytesdawehner
#5 drupal-1919016-5.patch851 bytesdawehner
#2 term-load-access-1919016-1.patch604 bytesBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Active » Needs review

Here is a patch. I'm assuming that we have no test coverage for this...

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
term-load-access-1919016-1.patch604 bytes

Grml.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Reviewed & tested by the community

Yeah, loading is a very wrong place, it's entity cache incompatible to do that.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work

No longer applies.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
drupal-1919016-5.patch851 bytes

We seem to be able to simplify this function a lot.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Status: Needs review » Reviewed & tested by the community

Yay for less code :)

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

Ha. Yes. For anyone that doesn't see it right away, all TermStorageController::buildQuery() was doing was adding the tag. If we aren't doing that anymore, we don't need to override the parent method.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Reviewed & tested by the community » Needs work

FYI it looks like this was there in the original conversion in #1361232: Make the taxonomy entities classed objects and part of @Berdir's original patch, so there wasn't a specific reason it was introduced that we need to worry about as far as I can tell. Edit: Hmm, do we have any test coverage around the term_access tag at all? I don't think we do since core doesn't do anything with it it. Edit 2: Except that Views is in core now, and Views does! See in core/modules/taxonomy/taxonomy.views.inc.

I also noticed in the process of digging back as to why these lines are there that core/lib/Drupal/Core/Entity/DatabaseStorageController.php has a reference to this method on the parent buildQuery() method, so we need to update that documentation as well.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
interdiff.txt940 bytes
drupal-1919016-9.patch1.75 KB

Good catch!

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Reviewed & tested by the community

Re-roll looks good.

Initially, there was also a translatable tag but I removed that in another issue, so now we can remove the whole function yes. I really want to remove that other example there as well :)

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

Let's add a followup to add some test coverage for the term access tag?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

#1947270: Add test coverage for the term_access query tag

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

#9: drupal-1919016-9.patch queued for re-testing.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +Quick fix
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

D'oh. Sorry, thought I had committed this long ago...

Committed and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.