-
Notifications
You must be signed in to change notification settings - Fork 347
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
Add event to allow inspecting and changing multipart responses #1074
Conversation
190e42c
to
292108e
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1074 +/- ##
============================================
+ Coverage 97.22% 97.24% +0.01%
Complexity 2834 2834
============================================
Files 175 175
Lines 9018 8843 -175
============================================
- Hits 8768 8599 -169
+ Misses 250 244 -6 ☔ View full report in Codecov by Sentry. |
e5366d0
to
01b428a
Compare
@icewind1991 Can I ask you for an example code snippet on how this event can be used? I'm missing some logic which tells the code on what to do after the event was take care of or not. THX |
My current use case for this is here: https://github.com/nextcloud/server/pull/10880/files#diff-a8ef54ad5b45404480feafc1de1eea28R62 where it stores a copy of the mutlipart listing result and limits the output returned. |
I see you are using it for caching the dataset for later reuse - got it. Why are you passing the properties by ref then? |
Because it also overwrites the returned properties to limit the number of results returned in the response. |
Got it - THX I was having trouble in the past as well with the missing capabilities to change the behavior of generateMultiStatus What about a more generic approach where the whole method can be overwritten? like emitting a generateMultiStatus event and in case not handled we fall back to the default implementation. My use case was to chnage the content type from xml to json ... @staabm what's our point on this? THX |
The ability to overwrite the method would also work yes, this was just the minimal approach I found |
@icewind1991 how do we continue with this one? |
01b428a
to
a9fe8fc
Compare
Rebased. I would still like to have this merged as I believe that it provides a useful option for plugin authors at minimal cost. |
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.
LGTM
I will have a look through other pending sabre
repo PRs over the next day or so, and probably make some releases "soon".
Ported to the major version 4 release series in PR #1549 |
Use case for this is allowing caching/pagination of multipart responses.