Problem/Motivation

The class docs for TaxonomyImageTest say:

> * Tests access checks of private image fields.

But this test coverage is already provided by FilePrivateTest in the file module and ImageFieldDisplayTest in the image module.

There's nothing special about taxonomy module in this case (or there shouldn't be!)

Steps to reproduce

Proposed resolution

Remove the test.

Remaining tasks

Check that ImageFieldDisplayTest covers everything in TaxonomyImageTest.

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#6 core-3305410-6.patch4.08 KB_shy
#3 core-3305410-3.patch4.04 KB_shy

Comments

joachim created an issue. See original summary.

joachim’s picture

joachim commented
Parent issue: #3305376: [Meta] Perform set-up tasks in Browser tests using API calls rather than browser requests »
_shy’s picture

_shy
Primary language Ukrainian
Location Ukraine, Lutsk 🇺🇦
commented
StatusFileSize
new core-3305410-3.patch4.04 KB

Checked ImageFieldDisplayTest class.
Functions testImageFieldFormattersPublic() and testImageFieldFormattersPrivate() checks the access for the image field in general. Meanwhile, TaxonomyImageTest did actually the same, but specially for the taxonomy image field.

I suppose, it's make sense to remove TaxonomyImageTest class. Added patch, just in case =)

_shy’s picture

_shy
Primary language Ukrainian
Location Ukraine, Lutsk 🇺🇦
commented
Status: Active » Needs review
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

Did some digging to find out why we have two tests.

TaxonomyImageTest was added in #1327224: Access denied to taxonomy term image where we also added taxonomy_file_download_access() as part of a bug fix. Then taxonomy_file_download_access() was refactored away again in #2078473: Use entity access API for checking access to private files to a more generic solution that covered all entity types, so I agree that this explicit test coverage for taxonomy can be removed.

_shy’s picture

_shy
Primary language Ukrainian
Location Ukraine, Lutsk 🇺🇦
commented
StatusFileSize
new core-3305410-6.patch4.08 KB

Patch update due to issue with applying.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Funnily enough there's a case to use this as a test for something else - but that will require moving to javascript and re-focusing the test so I think removing is fine. See #3277238: Fix \Drupal\taxonomy\Entity\Term::getName() to conform to the interface

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 0cf5ca15dc to 10.1.x and 0bd8f26502 to 10.0.x and b771071a47 to 9.5.x. Thanks!

As a test only change backported to 9.5.x

  • alexpott committed 0cf5ca1 on 10.1.x 
    Issue #3305410 by _shY, joachim, longwave: remove TaxonomyImageTest

  • alexpott committed 0bd8f26 on 10.0.x 
    Issue #3305410 by _shY, joachim, longwave: remove TaxonomyImageTest

(...

  • alexpott committed b771071 on 9.5.x 
    Issue #3305410 by _shY, joachim, longwave: remove TaxonomyImageTest

(...

Status: Fixed » Closed (fixed)

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