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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Refactor the plugin sub system #2426

Open
wants to merge 12 commits into
base: psr2
from

Conversation

Projects
None yet
4 participants
@splitbrain
Owner

splitbrain commented Jun 15, 2018

This moves all plugin related stuff to the dokuwiki\Extension namespace (dokuwiki\plugin is already reserved for the actual plugins).

  • move the plugin controller to the namespace
  • make the plugin controller a singleton so we don't need the global
  • deprecate the plugin utils
  • fix remaining PSR 2 problems in the Extension namespace
  • refactor plugin_getRequestAdminPlugin

I'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?

@splitbrain

This comment has been minimized.

Owner

splitbrain commented Jun 15, 2018

@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?

@@ -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 */

This comment has been minimized.

@Klap-in

Klap-in Jun 15, 2018

Collaborator

Is \dokuwiki\Plugin\ here kind of double?

@gamma

This comment has been minimized.

Contributor

gamma commented Jun 16, 2018

fix type hints
thos broke during refactoring
@splitbrain

This comment has been minimized.

Owner

splitbrain commented Jul 20, 2018

@gamma any news?

splitbrain added some commits Jul 20, 2018

removed get_directory() method from PluginController
This method did absolutely nothing and just returned the plugin name.
isEnabled instead of isDisabled
Boolean Methods with a negative name are a bit confusing. Getting false
for an enabled plugin? Better check for Enabled status.
@gamma

This comment has been minimized.

Contributor

gamma commented Jul 20, 2018

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.

splitbrain added some commits Jul 20, 2018

avoid asking an uninitialized event system for info
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.
made plugin controller a singleton
We want to reduce the global pollution
deprecate the pluginutils
Those where always just thin wrappers around the Plugin Controller.
Fixed remote plugin tests
Mocking the plugin controller doesn't work like before anymore, because
of the singleton mechanim.
@splitbrain

This comment has been minimized.

Owner

splitbrain commented Jul 21, 2018

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 parserutils_set_metadata_during_rendering_test this wouldn't work so well...

Maybe instead of making the plugin controller a singleton, we should use a simple dependency container instead. $container->plugincontroller would give us the controller from everywhere while making sure to always pass the same instance. I guess this one would be global again?

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?

@micgro42 @Chris--S @Klap-in opinions?

protected static $instance;
/** The different types of plugins DokuWiki supports */
const PLUGIN_TYPES = array('auth', 'admin','syntax','action','renderer', 'helper','remote');

This comment has been minimized.

@micgro42

micgro42 Oct 13, 2018

Collaborator

Shouldn't this list include cli?

@micgro42

This comment has been minimized.

Collaborator

micgro42 commented Oct 14, 2018

@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.

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