Problem/Motivation

The function signature for clearstatcache is clearstatcache(bool $clear_realpath_cache = false, string $filename = ''): void but in FileFieldRevisionTest::testRevisions it is called 4 times with only a string file URI.

Since these filename strings would be truthy that would be equivalent of passing TRUE for the $clear_realpath_cache argument, meaning we are clearing the entire stat cache and realpath cache 4 times!

Moreover, there is a todo comment above these calls that suggests the calls should not be necessary:

// TODO: This seems like a bug in File API. Clearing the stat cache should
// not be necessary here. The file really is deleted, but stream wrappers
// doesn't seem to think so unless we clear the PHP file stat() cache.

The test still passes after removing these calls, so that confirms they are not necessary.

Steps to reproduce

Proposed resolution

Remove calls to clearstatcache

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3417935

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:

Comments

mstrelan created an issue. See original summary.

mstrelan’s picture

Status: Active » Needs review
acbramley’s picture

Status: Needs review » Reviewed & tested by the community

Pipeline is green, test still makes sense without these lines (e.g the comment above the user deletion talks about checking the file, which is done below these removed lines). Don't see a reason to hold this up.

mstrelan’s picture

FWIW the TODO comment and clearstatcache call was first added in the file contrib module 14 years ago and moved to core in #391330: File Field for Core.

It was then updated to include the file URI in #1748880: Only clear the stat cache for the files we care about.

mstrelan’s picture

Issue summary: View changes

  • catch committed 1d3255c7 on 10.2.x
    Issue #3417935 by mstrelan, acbramley: Remove calls to clearstatcache in...

  • catch committed 9d0eaceb on 11.x
    Issue #3417935 by mstrelan, acbramley: Remove calls to clearstatcache in...
catch’s picture

Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x and cherry-picked to 10.2.x, thanks! Amazing what you find sometimes.

Status: Fixed » Closed (fixed)

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