Problem/Motivation

Currently \Drupal\Tests\file\Kernel\FileSaveUploadTest::setUp() creates a file to test uploads using \file_put_contents('test.bbb', 'test');

This creates a left over file test.bbb in the app root when tests are done.

Steps to reproduce

Proposed resolution

Write the file to the OS temporary directory instead.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3455183

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

kim.pepper created an issue. See original summary.

kim.pepper opened merge request !8438

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Active » Needs review
mstrelan’s picture

mstrelan commented
Status: Needs review » Needs work

This was also spotted in #3445847: PHPUnit 10 behaves differently when invoked outside web root and I added a tearDown method to remove the created file. We can remove that tearDown method now in this issue.

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review

Ah cross-post, disregard #4.

smustgrave’s picture

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

Seems like a good refactor.

mondrake’s picture

mondrake
Location 🇮🇹
commented

It’s a Kernel test, why not using vfs instead?

  • xjm committed 62f9af40 on 11.x 
    Issue #3455183 by kim.pepper, mstrelan, mondrake: FileSaveUploadTest...

  • xjm committed f03fb0f6 on 11.0.x 
    Issue #3455183 by kim.pepper, mstrelan, mondrake: FileSaveUploadTest...
xjm’s picture

xjm
she/her
Primary language English
commented
Version: 11.x-dev » 10.4.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Definitely much better.

Committed to 11.x and 11.0.x. There's a merge conflict on cherry-pick to 10.4.x, so we would need a different backport version.

Thanks!

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

pooja_sharma opened merge request !9371

pooja_sharma’s picture

pooja_sharma commented

Ported the changes to 10.4.x, PLease review MR 9371

smustgrave’s picture

smustgrave commented
Version: 10.4.x-dev » 10.5.x-dev

Probably missed 10.4 but could do 10.5

smustgrave closed merge request !8438

smustgrave’s picture

smustgrave commented
Status: Patch (to be ported) » Reviewed & tested by the community

Pretty clean backport

  • alexpott committed 8ee22c46 on 10.5.x 
    Issue #3455183 by kim.pepper, mstrelan, xjm, mondrake:...

  • alexpott committed 76c2d39a on 10.6.x 
    Issue #3455183 by kim.pepper, mstrelan, xjm, mondrake:...
alexpott’s picture

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

Seems fair enough to backport to 10.5.x.

Committed and pushed 76c2d39afb4 to 10.6.x and 8ee22c46ac4 to 10.5.x. Thanks!

I think this is a bug. Tests should not leave files around.

quietone closed merge request !9371

Status: Fixed » Closed (fixed)

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