-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WebDAV - use file/folder name for dav:displayname #34508
Conversation
Signed-off-by: Dariusz Olszewski <starypatyk@users.noreply.github.com>
* Disable modification of the displayname property for files and | ||
* folders via PROPPATCH. See PROPFIND for more information. | ||
*/ | ||
$propPatch->handle(self::DISPLAYNAME_PROPERTYNAME, function ($displayName) { |
Check notice
Code scanning / Psalm
MissingClosureParamType
Let us check our clients:
|
For the background: before this PR it would be possible for a user to manually send a PROPPATCH to set an arbitrary value for "dav:displayname". If the clients were using it, it would then affect how the file name is displayed there. That property is not used in the web UI, so the behavior would be inconsistent anyway. I just wonder if there was a past use case that existed for that property ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 as discussed
iOS use it but ... if not more present is not a problem, I can replace it ... please ping me if we want remove it |
@starypatyk and me was thinking about card dav / cal dav, where a display name is different from actual "file" representation. --> if display-name works and shows 1:1 as last part of href, it is fine :) |
desktop client is not using |
I did very brief checks of some clients with the proposed changes applied.
|
@matthieugallien does it mean Windows will try to store those with a PROPPATCH dav:displayname ? |
ok, seems @starypatyk already tested on Windows so I guess we're good to go |
second review please @CarlSchwan @artonge @come-nc |
/backport to stable25 |
hello @starypatyk , |
Hi @DaphneMuller, The primary reason for submitting this PR was improving performance of the WebDAV queries. The change of behaviour was a side-effect. Previously the dav:displayName property could be set for files and folders by a third-party client to any value - independent of the the file/folder name. Currently this property is always the same as the file/folder name and cannot be changed using the PROPPATCH request. Having separate file name and displayName seems to be a very unusual use case. We are not aware of any WebDAV client doing such things. The above might be mentioned in the developer documentation in Supported DAV properties section. However, when I read this section now, the description there seems to be closer to the current behaviour than to the previous one. @PVince81 - any additional comments? |
hello @starypatyk , thank you so much for your lightning fast and helpful reply! It sounds like the documentation is already pretty much done there. Thanks again for your great work. For now I will then remove the 'needs documentation' tag. Any objections @artonge @come-nc ? |
Fix #17778
Follow-up to #34471 - also found during investigation of nextcloud/android#10783.
I have found that:
This is an attempt to improve performance by handling the dav:displayname property in the FilesPlugin and avoiding the fallback.
Performance results - 331 files in a folder, assuming #34471 is in place, timings are not very consistent:
Additional notes: