fix: normalise trashbin-original-location PROPFIND response (full-ci)#41649
fix: normalise trashbin-original-location PROPFIND response (full-ci)#41649phil-davis wants to merge 1 commit into
Conversation
|
I have run full-ci because we want to know that all the API acceptance tests still pass. |
DeepDiver1975
left a comment
There was a problem hiding this comment.
🤖 Automated review by Claude Code review agent.
Thanks for tackling this — it targets the exact inconsistency the triage flagged in #39337 (the oc:trashbin-original-location leading-slash mismatch: the deleter's entry was stored relative/no-slash via move()+pathinfo()['dirname'], while the owner's copy used dirname()/absolute, and getOriginalLocation() reflected that verbatim). Normalising the PROPFIND output to a single, stable leading-slash form is the right direction.
Verdict: changes-requested — the fix is incomplete and CI proves it.
Root-cause coverage
The change in AbstractTrashBinNode::getOriginalLocation():
- $originalPath = $location . '/' . $originalPath;
+ $originalPath = '/' . ltrim($location, '/') . '/' . $originalPath;correctly normalises the non-root case: regardless of whether $location arrives with a leading slash (owner copy) or without (deleter copy), the output now gets exactly one leading slash. Good.
But the normalisation only runs inside the if ($location !== '.') branch. For a file deleted from the root ($location === '.'), that branch is skipped and the method returns the bare $originalPath (e.g. textfile0.txt) with no leading slash. So the root-level case — the most common one — is still un-normalised.
CI failure (directly caused by this PR)
Two required API Smoke Test shards are failing, with 8 trashbin scenarios failing:
Failed step: But as "Alice" the file with original path "/textfile0.txt" should exist in the trashbin
File previously located at /textfile0.txt wasn't found in the trashbin of user Alice
This PR also tightens the test helper TrashbinContext::isInTrash() to an exact comparison by removing the ltrim() tolerance on both sides:
- $originalPath = \ltrim($originalPath, '/');
- if (... \ltrim($entry['original-location'], '/') === $originalPath) {
+ if (... $entry['original-location'] === $originalPath) {The feature files now assert against /textfile0.txt (leading slash), but for root-deleted files the PROPFIND value is still textfile0.txt (no slash) because of the $location === '.' gap above — so the exact match fails. The test tightening and the production fix are out of sync for the root case.
Suggested fix
Apply the leading slash unconditionally (or handle the . case), e.g.:
$originalPath = '/' . \ltrim($originalPath, '/');
if ($location !== '.') {
$originalPath = '/' . \ltrim($location, '/') . \rtrim($originalPath, '/') ... ;
}or simply normalise the final return value once: return '/' . ltrim($originalPath, '/');. Then re-run the trashbin smoke suites until green.
Changelog
changelog/unreleased/41649 is present and well-formed (Bugfix: type, present-tense summary <80 chars, descriptive body, issue + PR URLs including the #39337 link).
Summary
- Resolves #39337's intent, but the fix misses the
$location === '.'(root-deleted file) path. - Required checks are failing (8 trashbin acceptance scenarios) as a direct result.
- Please slash the root case and get CI green; happy to re-review.
In certain situations the trashbin-original-location in a PROPFIND response did not contain a leading slash. This change ensures that a leading slash is always returned.
#39337
#41649
Fixes #39337
The test code changes related to this issue from old PR #39336 have also been reverted in this PR.