WIP: Refactor the plugin sub system #2426
Conversation
|
@Chris--S we currently have a way to overwrite which class is used as the plugin controller: https://github.com/splitbrain/dokuwiki/blob/master/inc/init.php#L182 As far as I can see only i-net-software/dokuwiki-plugin-siteexport by @gamma uses this. I wonder if we really need it. @gamma can you explain why you overwrite the plugin controller with your own class? |
inc/Remote/ApiCore.php
Outdated
| @@ -594,7 +594,7 @@ public function deleteUsers($usernames) | |||
| if (!auth_isadmin()) { | |||
| throw new AccessDeniedException('Only admins are allowed to delete users', 114); | |||
| } | |||
| /** @var DokuWiki_Auth_Plugin $auth */ | |||
| /** @var \dokuwiki\Plugin\\dokuwiki\Extension\AuthPlugin $auth */ | |||
There was a problem hiding this comment.
Is \dokuwiki\Plugin\ here kind of double?
|
It has something to do with dynamically disabling plugins. But I’ll have to dig into it by the end of next week, because I’m on vacation right now ;)
Lg,
Gerry.
…Sent from my iPhone5
On 15. Jun 2018, at 19:28, Andreas Gohr ***@***.***> wrote:
@Chris--S we currently have a way to overwrite which class is used as the plugin controller:
https://github.com/splitbrain/dokuwiki/blob/master/inc/init.php#L182
As far as I can see only i-net-software/dokuwiki-plugin-siteexport by @gamma uses this. I wonder if we really need it. @gamma can you explain why you overwrite the plugin controller with your own class?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
thos broke during refactoring
|
@gamma any news? |
This method did absolutely nothing and just returned the plugin name.
Boolean Methods with a negative name are a bit confusing. Getting false for an enabled plugin? Better check for Enabled status.
|
Creating a custom implementation of the class allowed to enable/disable plugins temporarily without writing these changes to the disk. This allowed the siteexport plugin to make the JS and CSS a lot smaller. As far es I can see this only applies to JS and CSS and afaik we have a (new) mechanism to include/exclude styles. I think it should be possible to modify siteexport accordingly ... so, please go forward with your approach. |
I deprecated some stuff that is used during the event/plugin initialization. The event in dbg_deprecated can be triggered then and everything breaks. We don't want that. Of course the deprecated stuff has to be adjusted, too.
We want to reduce the global pollution
Those where always just thin wrappers around the Plugin Controller.
Mocking the plugin controller doesn't work like before anymore, because of the singleton mechanim.
|
Working some more on this I wonder if the Singleton idea is a good one. I'm hitting some road block on trying to fix the tests. There are some tests that relied on mocking parts of the plugin controller to load custom pseudo plugins. This was relatively easy because the plugin controller could simply be replaced in the global. I fixed the problem for the remote tests by moving the test code to our testing plugin (dc0f84f). However for the failing Maybe instead of making the plugin controller a singleton, we should use a simple dependency container instead. sigh I think I got somewhat derailed here... all I wanted was doing some PSR2 cleanup. Maybe I should roll back some of the changes for now? |
| protected static $instance; | ||
|
|
||
| /** The different types of plugins DokuWiki supports */ | ||
| const PLUGIN_TYPES = array('auth', 'admin','syntax','action','renderer', 'helper','remote'); |
There was a problem hiding this comment.
Shouldn't this list include cli?
|
@splitbrain Sorry, for the late feedback. A container might indeed be a solution. However, we are likely to use it not like a dependency injection container and more like a service locator, which, as I understand, is considered an anti-pattern. Nonetheless this seems to be better than that singleton and we can extend it in the future. On the other hand, switching to a container-based architecture might be a more fundamental step and it might become difficult to adjust design decisions here without breaking things. So I see the advantage of not doing this in a branch about PSR-2 adjustments but in its own PR, in order to give it more publicity and to get more feedback/reviews. |
This moves all plugin related stuff to the
dokuwiki\Extensionnamespace (dokuwiki\pluginis already reserved for the actual plugins).plugin_getRequestAdminPluginI'm not very happy with having to to use the legacy class names in the Action plugin type hints. But changing them again would break signature checks (again). Ideally we would not have used type hinted signatures at all 😐
I'm unsure if we should deprecate
plugin_load()- it's used like a million times in our and 3rd party code, so maybe we should just keep it forever?