textpattern / textpattern

A flexible, elegant, fast and easy-to-use content management system written in PHP.
https://textpattern.com
GNU General Public License v2.0
788 stars 112 forks source link

Multi-site plugins directory: central or per-site as default? #1775

Closed jools-r closed 2 years ago

jools-r commented 2 years ago

Not really a bug or a feature request but a question:

Currently if you set up a multi-site installation, images, files and themes are stored in the /sites/sitename/public directory. However, if you install a plugin, it is saved to the central /textpattern/plugins directory that serves all multi-site sites. This gives you a central repository for plugins but also means that any alterations or interactions act on all sites.

My questions is: might it be better to save this in the sites admin directory by default?

Currently PLUGINPATH is a constant set in lines 302-304 of /textpattern/lib/constants.php.

It is wrapped in if (!defined… so I assume this means it can be overwritten by adding the line:

define('PLUGINPATH', $txpcfg['multisite_root_path'] . '/admin/plugins');

to the end of config.php ($txpcfg['multisite_root_path'] is defined in config.php on multi-site installations).

In /vendors/textpattern/DB/Core.php we set path variables depending on the existence of $txpcfg['multisite_root_path']. Could we not do the same thing for PLUGINPATH here?

I think there are very few multisite installations out there, and the writing of plugins to the /plugins directory is comparatively recent so the impact of a change with respect to backwards compatibility is probably minimal. And those cases could presumably be fixed with a DEFINE statement in config.php.

What do you think?

jools-r commented 2 years ago

For example, this works fine:

if (!defined('PLUGINPATH')) {
    global $txpcfg;
    $admin_path = (isset($txpcfg['multisite_root_path'])) ? $txpcfg['multisite_root_path'].DS.'admin' : txpath;
    define('PLUGINPATH', $admin_path.DS.'plugins');
}

(improvements welcome)

If this is adopted, we also need to add an empty /plugins folder in /sites/site1/admin/ to the repo.

Bloke commented 2 years ago

Yes, we should do that. I dislike the location of plugins inside /textpattern anyway and I swear we had an Issue open somewhere discussing a better directory structure, but maybe it was in the forum. Anyway, yep, can't see any downside to this.

jools-r commented 2 years ago

Thanks, that'd be great!

Yes, in the forum: I think from here onwards. The issue was the potential accidental overwriting of the /plugins (and config.php) when upgrading. In the multisite setup config.php ist out of the way in the /private folder...

petecooper commented 2 years ago

I swear we had an Issue open somewhere discussing a better directory structure

@Bloke – https://github.com/textpattern/textpattern/issues/1440 ?

Bloke commented 2 years ago

Ah, thank you. Knew it was somewhere.

Bloke commented 2 years ago

@jools-r Would you like to push this through as a PR to the 4.8.8 codebase and add the empty plugins dir (with .htaccess-dist placeholder)?

petecooper commented 2 years ago

re: https://github.com/textpattern/textpattern/issues/1775#issuecomment-1018863568

In layman's terms, that provides an extra directive in config.php for plugins to be optionally routed to another directory, right? Nothing forced?

Bloke commented 2 years ago

As far as I can make out, yes. Haven't traced it through though.

petecooper commented 2 years ago

We can rework multisite doc(s) after launch easily enough. Pinging https://github.com/textpattern/textpattern.github.io/issues/195 for reference.

jools-r commented 2 years ago

re: #1775 (comment)

In layman's terms, that provides an extra directive in config.php for plugins to be optionally routed to another directory, right? Nothing forced?

Not quite. My revision of the multisite setup a few years back predates the saving out of plugins to the /plugins folder. Files and themes are saved to the respective multisite's folder but currently plugins are not; they are saved to the central base textpattern folder.

At present, the multisite setup routine already adds $txpcfg['multisite_root_path'] to config.php. This $txpcfg key does not exist in regular installations. The core checks if it exists and then sets the various path prefs accordingly, either to the multisite_root_path + /public or /admin or to txpath as usual. We just forgot to do that when adding save to /plugins.

My proposal in #1775 just adds that check so that the plugins folder is located within the respective site's folder. That does change the prevailing behaviour, though this behaviour has only existed since plugin code was saved out to the /plugins folder (v4.8.?).

I've no idea how many multisite installation have been made since then but I would guess very few, so I think the potential impact is minimal. In addition, should a site be affected, they have two easy options:

define('PLUGINPATH', $txpcfg['txpath'] . '/plugins');
jools-r commented 2 years ago

@jools-r Would you like to push this through as a PR to the 4.8.8 codebase and add the empty plugins dir (with .htaccess-dist placeholder)?

Will do.

petecooper commented 2 years ago

Closing now, pending review of https://github.com/textpattern/textpattern/pull/1776 – thanks @jools-r!