Problem/Motivation

#1452100: Private file download returns access denied, when file attached to revision other than current recently was committed but only contains functional test coverage for uploading files to nodes. While this does test the FileReferenceResolver service I think it would be wise to add additional, specific test coverage for the service. This will give us greater confidence to refactor this service.

Also seems like we may be missing coverage for some of the nuanced revision and translation handling, the service is rather complex.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3590777

Command icon Show commands

Start 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:

About issue forks

Comments

acbramley created an issue. See original summary.

mohit_aghera made their first commit to this issue’s fork.

mohit_aghera opened merge request !15976

mohit_aghera’s picture

mohit_aghera commented
Status: Active » Needs review
smustgrave’s picture

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

From what I can tell this properly tests the file on different revisions. Wasn't sure what else to look for but LGTM.

mstrelan’s picture

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

Left some questions, feel free to restore the RTBC status once answered.

smustgrave’s picture

smustgrave commented

@mohit_aghera thoughts?

mohit_aghera’s picture

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

Resolved both the comments.
Moving back to RTBC

  • longwave committed 64388892 on 11.x 
    test: #3590777 Add kernel test coverage for FileReferenceResolver

By:...

  • longwave committed 0d32f86c on main 
    test: #3590777 Add kernel test coverage for FileReferenceResolver

By:...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Backported to 11.x as the class was also added there.

Committed and pushed 0d32f86cfe5 to main and 64388892dc9 to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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