Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
Under certain conditions the Filehash service can enter an endless loop scenario as it attempts to generate a hash for a file that does not exist repetitively.
Steps to reproduce
- Have a separate module that has entity CRUD hook implementations that load up entities.
- Save a file entity whose URI does not exist on the filesystem.
- Endlessly loop.
An example implementation in a contrib module can be viewed at https://git.drupalcode.org/project/content_sync/-/blob/2bfa12c22b63b35ff.... Will be committing a tests only patch that exhibits this behavior.
Proposed resolution
Don't attempt to hash a file that does not exist or is not readable.
Remaining tasks
User interface changes
None.
API changes
None.
Data model changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | 3309518-10.patch | 7.1 KB | mfb |
Issue fork filehash-3309518
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:
- 3309518-endless-loop-possible
changes, plain diff MR !2
Comments
Comment #2
jordandukart commentedSo while making the MR noticed https://www.drupal.org/project/filehash/issues/3276224 and how the existing test there was added to ensure a warning was outputted when this specific scenario was hit. That test will currently fail given the change in behavior in the shouldHash portion of the service.
@mfb, thoughts on how you'd like to proceed here?
Comment #4
mfbPrevious versions of this module called is_readable() but I thought it would be helpful to log the error re: unreadable file. Also, I actually played with a file URI that *couldn't* call is_readable() - i.e. didn't have the ability to stat the URI - although admittedly that was more of a construct in my dev sandbox than something I ran across in the real world.
Endless loop sounds bad, guess I need to look at why/how that happens.
Comment #5
mfbOk I was able to reproduce with an unreadable file and your test module. Theoretically, especially for remote files, there could be a read failure even if a file is readable. So maybe it's a good idea to prevent this auto-hashing behavior from being reattempted for a given file? Something like this patch?
Comment #6
jordandukart commentedThis fixes the issue at hand and retains the existing warning behavior which is a plus in my book! Behavior where this was popping up was with a remote filesystem using Flysystem just for reference.
Only comment I’d have is whether using MemoryCache (https://www.drupal.org/node/2973262) instead of utilizing a static is worthwhile, though could be overkill given what is actually being tracked in this case. That being said wouldn't considered it being a blocker on my end.
Comment #7
mfbYes it'd be cleaner to not store state in this service, and there is the possibility of long-running processes, so I think MemoryCache is correct.
Comment #8
mfbAdding a memory cache service, and your test from the MR (renamed to end with "Test")
Comment #9
mfbJust realized this branch still claims to work with Drupal 8.5 - we need to bump it up to 8.6 to use MemoryCache.
Comment #10
mfbOops this core_version_requirement causes an InfoParserException - it has to say ^8 not ^8.6 - and apparently there is no test coverage for this for some reason?
Comment #11
jordandukart commentedTested the updated patch in #10, looking good to me.
Comment #12
mfb@JordanDukart thanks for testing
Comment #14
mfb