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

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
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
commented
StatusFileSize
new term-load-access-1919016-1.patch604 bytes

Grml.

chx’s picture

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 🇨🇦
commented
Status: Reviewed & tested by the community » Needs work

No longer applies.

dawehner’s picture

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

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

amateescu’s picture

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

Yay for less code :)

xjm’s picture

xjm
she/her
Primary language English
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
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
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt940 bytes
new drupal-1919016-9.patch1.75 KB

Good catch!

berdir’s picture

berdir
Primary language German
Location Switzerland
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
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
commented

#1947270: Add test coverage for the term_access query tag

xjm’s picture

xjm
she/her
Primary language English
commented

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

xjm’s picture

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

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
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.