-
Notifications
You must be signed in to change notification settings - Fork 97
fix(dav): Register DAV plugins via SabrePluginAddEvent #4234
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
base: master
Are you sure you want to change the base?
Conversation
ebfa193 to
daa6ca6
Compare
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Replaced with event listeners Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
08141d3 to
d156e21
Compare
come-nc
left a comment
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.
I’m afraid about this because there are code paths which loads plugins from PluginManager::getAppPlugins which are not triggering SabrePluginAddEvent.
Also there is the ServerFactory using OCP\SabrePluginEvent 🤷
So I’d favor a cleanup of sabre server building on server side before migrating applications, unless you’re sure the plugins from groupfolders are only needed in the code paths using SabrePluginAddEvent ?
| # php-version: ${{ steps.versions.outputs.php-available }} | ||
| php-version: '8.2' |
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.
why is this changed?
A refactor that modernizes slightly, by switching SabreDAV plugin loading over to the event listener. Drops Sabre plugin management declarations from
appinfo/info.xml.Doesn't make any real difference at the moment, but less future code debt.
Misc:
TODO:
Split out Psalm php version range temporary lock into separate PRDrop Psalm php version lock once split out is merged