Skip to content

Conversation

@susnux
Copy link
Contributor

@susnux susnux commented Oct 25, 2023

Currently Sabre\DAV does not support the Depth header on COPY and MOVE.
But the RFC4918 requires servers to do so, the header is not required but:

  • If the header is missing 'infinity' shall be assumed -> This is how we handle every request currently
  • For MOVE the header, if available, must be 'infinity' -> We just have to check the header and throw bad request otherwise
  • For COPY the header may 'infinity' or a positive integer including 0
    • The depths defines how many levels of children shall be copied
    • If the depth is zero only the collection and its properties are copied, but not any children

So especially the COPY method is currently broken, we noticed this when using the WebDAV litmus test bench (version 0.14) which checks for shallow copies (meaning copy a collection with depth 0).

This PR fixes all of this points, while trying to keep backwards compatible. Of cause any using application that implements ICopyTarget::copyInto is still broken and need to adapt to the depth change (it continues to work as before, but the bug fix does not propagate).

Reference from the RFC:

  • The requirement on the server support of COPY Depth 'infinity' and '0': rfc4918#section-9.8.3
  • For MOVE only Depth: infinity is allowed: rfc4918#section-9.9.2

@susnux susnux requested a review from DeepDiver1975 February 21, 2024 17:08
@susnux
Copy link
Contributor Author

susnux commented Apr 4, 2024

ping again, also cc @phil-davis

Would be great to get this fixed so sabre passes the litmus test v15 ( https://github.com/notroj/litmus previously http://www.webdav.org/neon/litmus/ )

@phil-davis phil-davis self-assigned this Apr 18, 2024
@susnux
Copy link
Contributor Author

susnux commented Aug 13, 2024

Anything I can help with to get this reviewed? (I do not want to bug you but this would be a great to get fixed :) ).

@DeepDiver1975
Copy link
Member

I miss the ci status - what's going on here?

@DeepDiver1975
Copy link
Member

I miss the ci status - what's going on here?

@susnux can I ask you to rebase this pull request ... THX

@susnux
Copy link
Contributor Author

susnux commented Sep 5, 2024

can I ask you to rebase this pull request ... THX

@DeepDiver1975 sure, done!

Noticed I had some code style issues / compatibility issues because I was using PHP 8+ syntax and tests use PHP 7.4. This is now fixed.

@codecov
Copy link

codecov bot commented Sep 5, 2024

Codecov Report

Attention: Patch coverage is 96.42857% with 1 line in your changes missing coverage. Please review.

Project coverage is 97.24%. Comparing base (ed901a9) to head (1af1212).
Report is 9 commits behind head on master.

Files with missing lines Patch % Lines
lib/DAV/Tree.php 93.33% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1495      +/-   ##
============================================
+ Coverage     97.23%   97.24%   +0.01%     
- Complexity     2836     2842       +6     
============================================
  Files           175      175              
  Lines          9028     9040      +12     
============================================
+ Hits           8778     8791      +13     
+ Misses          250      249       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@susnux
Copy link
Contributor Author

susnux commented Jan 29, 2025

Noticed it is still open, is there anything I can help with?

@kesselb
Copy link
Contributor

kesselb commented Jun 26, 2025

cc @susnux can you rebase, to have a recent ci run?

susnux added 3 commits June 26, 2025 13:48
Only `Depth: infinity` is allowed, ref: rfc4918#section-9.9.2

Signed-off-by: Ferdinand Thiessen <[email protected]>
1. According to the RFC[1] servers **must** support `Depth` 'infinity' and 0.
2. And COPY method on a collection without a Depth header MUST act as if
a Depth header with value "infinity" was included.

[1] rfc4918#section-9.8.3

Signed-off-by: Ferdinand Thiessen <[email protected]>
Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux susnux force-pushed the fix/handle-depth branch from e911e4c to 7930b55 Compare June 26, 2025 11:48
@susnux
Copy link
Contributor Author

susnux commented Jun 26, 2025

can you rebase, to have a recent ci run?

Sure, done

@susnux susnux requested a review from kesselb June 26, 2025 16:01
@susnux susnux force-pushed the fix/handle-depth branch from 46171cb to b580c46 Compare June 26, 2025 16:02
@susnux susnux force-pushed the fix/handle-depth branch from b580c46 to 1af1212 Compare June 26, 2025 16:18
@susnux
Copy link
Contributor Author

susnux commented Jun 26, 2025

@kesselb I pushed a commit solving your comments

@susnux
Copy link
Contributor Author

susnux commented Oct 10, 2025

@phil-davis anything else to do, to get this PR merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants