Skip to content
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

Pico 2.0 : plugins subdirectory handling #402

Closed
nliautaud opened this issue Nov 6, 2017 · 8 comments
Closed

Pico 2.0 : plugins subdirectory handling #402

nliautaud opened this issue Nov 6, 2017 · 8 comments

Comments

@nliautaud
Copy link

nliautaud commented Nov 6, 2017

Restricting plugins handling to plugins/PluginName.php and plugins/PluginName/PluginName.php, as defined in Pico 2.0 #401 #334 to resolve #203, may be a bit restricting. Some plugins could need different strategies and structures in their own directories, especially if they aren't entirely tied to Pico. See p01contact as an example.

A solution may be to skip silently plugins directories without the required plugins/PluginName/PluginName.php file instead of throwing a RuntimeException / File not found.
In that scenario, plugins could choose to use plugins/PluginName.php as a more flexible Pico-specific handle to their custom subdirectory.

@PhrozenByte
Copy link
Collaborator

You need a Pico-specific plugin class in any case, so how about simply putting said class in the plugins/MyPicoPlugin/MyPicoPlugin.php file? You can then do your setup (like registering an autoloader or including other files manually; you can even load other plugins there using the Pico::loadPlugin() method) in the MyPicoPlugin::__construct() method of the class. Including arbitrary files is unexpected and pretty bad behavior - not to mention the negative performance impact.

With Pico 2.0 we're trying to overhaul how plugins and themes are managed (also refer to Pico's picocms/composer-installer). Thus we made some BC-breaking decision for the future. These decisions are the main reason why we've released this as Pico 2.0 and not Pico 1.1 (what was the original plan, that's the reason why the branch is named pico-1.1).

@nliautaud
Copy link
Author

nliautaud commented Nov 6, 2017

Using plugins/MyPicoPlugin/MyPicoPlugin.php as a required Pico-specific handle would require CMS-agnostic plugins to implement a Pico specific build and release process. Allowing a subdirectory without this file, and lean on an external Pico specific handling file allow system-agnostic distribution without inner changes or specific releases, for example:

plugins/
    ThePlugin
        app/
            Main.php
    ThePlugin_Pico.php

other CMS imaginary strategy : 

plugins/
    ThePlugin_OtherCms.php
ThePlugin
    app/
        Main.php

It seems to me that not firing an exception on missing file if there is a subdirectory without this particular file, and lean on the existing behavior on looking for a specific plugins/MyPicoPlugin.php file would not impact performance, not include any arbitrary file or change Pico strategy.

@PhrozenByte
Copy link
Collaborator

PhrozenByte commented Nov 6, 2017

Can you please elaborate on why this would require a Pico-specific build and release process? As far as I can see you just have to move plugins/ThePlugin_Pico.php to plugins/ThePlugin/ThePlugin.php. If ThePlugin.php is already used by another class, simply tell your users to rename the ThePlugin directory to ThePicoPlugin and use ThePicoPlugin.php.

You're definitely right that not firing an exception has no performance impact, I was rather referring to Pico's old behavior including arbitrary files. However, I still believe that being more explicit here and throwing an exception is better than silently ignoring it. Additionally we don't want "decoupled" plugin files and directories: everything that is related to a plugin should either be in a single file or go into a single directory. Everything else just confuses users.

Explicitly failing if something goes wrong has another advantage: Just think of a plugin that limits access to specific files/paths (you created such a plugin, did you? 😉). I'm sure the admin wants to know when this plugin couldn't be loaded for some reason (e.g. because he accidentally named the directory PicoUser and not PicoUsers after an update).

@nliautaud
Copy link
Author

nliautaud commented Nov 6, 2017

The manipulation that you describe is what I would encapsulate in a specific build. Document it and require the user to do it is totally a possibility thus (and I'll probably do that myself), but such operations are always altering the user experience so I'm opening the discussion about reducing additional install steps in that case.

I hear you about non-decoupled and explicitly failing (instead of a warning for example). It's all about opiniated strategy vs flexible one, and I'm okay with both. 🙂

In that case I wanted to raise that these arbitrary requirements will depreciate a bit the user (or plugin developer) experience of the agnostic plugins, that will probably make them less common or reduce their impact in opposition to those only packaged for these requirements.

There are other solutions that I'm skipping because not entirely satisfying, like allowing the configuration of the plugin handler path (an additional install step again), or targeting plugin files that starts with the directory name (back to a less clean include strategy), etc.

@PhrozenByte
Copy link
Collaborator

I really can't see any problem here: Simply create a plugins/ThePlugin/ThePlugin.php and you're good to go. I don't think that there's a single multi-CMS plugin out there that can't conform to this absolutely simple requirement (including p01contact). This is no rocket science...

I can't understand why you're tempting providence by using strong words like "depreciating the user experience" - especially because I truly believe that handling just a single file or directory per plugin, and not doing unexpected things like silently ignoring plugins (what might even be a security issue), are strong improvements towards user experience.

Furthermore, please note that Pico has no "own" UI. We can't display warnings. We just can bail out - what has the big advantage that it's impossible to miss.

@nliautaud
Copy link
Author

I'm not pointing any problem here. Seeing that the 2.0 plugin handling require a slightly more restrictive structure than 1.0, I just wanted to know about the possibility to allow the handler file next to the plugin directory, and not considering directories only as plugins self-contained bundles.

I thought that it would be imaginable, without creating bad behaviors, performance pitfalls, unexpected behaviors or ignoring plugins, hiding errors or using UIs (I just mentioned the idea of php warnings :/).

As I said I heard and respect your choice to allow only one-file/one-dir structures (which is the point closing my suggestion), and noted that it would a bit, slightly depreciate the installation process of a plugin not pre-bundled that way. I totally agree that one-file/one-dir is simpler too...

Sorry for the intervention, and no words were strong here so sorry if it wasn't clear.

@PhrozenByte
Copy link
Collaborator

PhrozenByte commented Nov 6, 2017

Just to make this clear, this was no termination of discussion from my side, I'm still open to suggestions. I really appreciate feedback!

I understand your point: We're planning (a beta is no final release, there's still some room for changes) to introduce pretty strict requirements here (although "strict" doesn't necessarily mean "complex").

Unfortunately I can't see a solution, we've conflicting goals here: Silently ignoring files or directories in the plugins/ dir is a absolute No-Go. We want a more reliable (in terms of "no unexpected behavior") plugin infrastructure, since some plugins do pretty important stuff, even security-related stuff like access control. So we must either ensure that it is impossible to miss a plugin (e.g. due to a typo in the directory name), or tell users what is going on. One solution to the latter might be to issue some kind of warning (no PHP warning since this wouldn't make any difference to throwing a exception), but the problem is that Pico has no way to actually display it somewhere. Pico has no "own" UI, all themes must be able to display these warnings - and we have no control over themes whatsoever. So to bail out by throwing a exception is our only solution. Thus all that remains is to ensure that it is impossible to miss a plugin. But how? We can't support non-self-containing plugins (like the file/directory structure you've mentioned in #402 (comment)) without ignoring files or directories in the plugins/ dir.

Besides all of that, we must keep in mind that we're talking about theory here - afaik there's not a single plugin that can't meet these requirements. This has a huge impact on the topic's importance and might lead to a fundamentally different weighting of said conflicting goals.

@nliautaud
Copy link
Author

To make this clear, I'm ok with your points 😉
The choice of strictly considering every directory as a plugin bundle is indeed reinforcing security; and the alternatives to the strict application of "if there is X directory, it should contain X.php handle" which respect the security requirements that I think of are equally complex to the user or unnecessary to the overall ecosystem.
I'll add new instructions for installing the contact plugin on Pico 2.0.

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

No branches or pull requests

2 participants