Local files linked in HTML (and possibly other references) are not being tracked since the release of beta 24.
Specifically, this commit altered the public file pattern in a way that seems to be incompatible with local file systems. For example, what used to be {^/?sites/default/files/} is now {^/sites/default/files//}. The double slash at the end results in no matching in the getFileFromPath method.
https://git.drupalcode.org/project/entity_usage/-/commit/571be2c4ff7add4...
Track HTML links and insert a link to a public file on a local file system into a text field on a node. Saving the node does not save any tracking for the target file.
Revert the commit in question while a new approach, with better test coverage, is added.
TBD
None
None
None
| Comment | File | Size | Author |
|---|---|---|---|
| #3 | entity_usage-3521603-3.patch | 1.23 KB | john.oltman |
Show commandsStart 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:
changes
Comments
Comment #2
john.oltman commentedDo not use
Comment #3
john.oltman commentedComment #4
john.oltman commentedComment #5
john.oltman commentedComment #6
ludo.rI confirm the issue and patch #3 solves the issue.
Comment #7
ludo.rComment #8
marcoscanoYour patch is only reverting what has been done in #3514883: Assertion fails when using external stream wrappers (e.g. s3fs) in PublicFileIntegration
Unless I'm reading things very wrong, wouldn't this fix be re-introducing the bug fixed there?
I believe we first need to make sure we have test coverage for both bugs, and then come up with a fix that is compatible with both of them.
Comment #9
john.oltman commentedThere is only one bug, not two. The bug in #3514883: Assertion fails when using external stream wrappers (e.g. s3fs) in PublicFileIntegration was not fixed properly, creating a regression covered by this issue. We need a solution to that bug that does not cause a regression, at which point both issues (the original bug issue, and this regression issue) will be solved.
Yes, applying the patch simply reverts the change that caused the regression, which is why I did not originally mark this issue as Ready for Review, @ludo.r did a few days ago. I agree that Needs Work is the correct status at this point. The next step is for someone to develop a fix for the original bug that does not cause a major regression. That fix would ideally be an MR attached to this issue.
Comment #12
codebymikey commentedCan someone making use of s3fs confirm that the current MR addresses their original bug?
This works for me, and should theoretically work for s3fs too, as the trailing
/is still persisted.Unless someone else has time to create a test for this, I think the MR should be quickly tested and merged, or the original commit reverted, as the module is currently in an unusable state at the moment for basic core functionality in order to cater for a contrib module, and has silently been for months now.
Tests can always be added later without closing the issue, but I believe site builders will also need to be made aware that their entity usage stats might currently be inaccurate and will need to be reindexed, as some of my sites heavily rely on the entity usage count to be accurate in order for users to confidently delete them, but that trust might now be eroded with this issue.
Comment #15
alexpottI've merged @codebymikey's MR as I agree this is the correct fix. Yes we definitely should add some test coverage for this feature so we don't break it again.