New plugin type setting...

net-carver · 2008-06-20 16:19:13

Hello,

The problem…

I’ve noticed that plugins that are marked as ‘admin’ or ‘library’ also get loaded on the public side. Whilst this is great for those plugins that have both admin and public functionality, it does mean that admin-only and library plugins get loaded on the public side.

Measurements…

If I switch a pretty minimal 4.0.5 install to debugging I get this resource usage…

<!-- Queries: 22 -->
<!-- Memory: 2218Kb, end of textpattern() -->

With rs_admin_db_manager…

<!-- Queries: 22 -->
<!-- Memory: 2360Kb, end of textpattern() -->

With a few other admin (only) plugins…

<!-- Queries: 23 -->
<!-- Memory: 2792Kb, end of textpattern() -->

As you can see, the public memory footprint, and sometimes the query count, increase even though these plugins only provide admin-side functionality. Each unused admin-only plugin also has it’s code eval’d by the php parser too. Potentially unnecessary bloat for the public interface.

What I’d like to see…

Plugin authors given the ability to write “admin-only” plugins that don’t increase memory/query footprint for the public interface.

How to get to that point is another matter entirely :)

Included below is a quick patch based on the introduction of a new plugn type of ‘3’ (admin-only) to the development version of txplib_misc.php. The patch modifies the load_plugins() function to be a little more selective on which plugins get loaded.

Index: textpattern/lib/txplib_misc.php
===================================================================
--- textpattern/lib/txplib_misc.php	(revision 2913)
+++ textpattern/lib/txplib_misc.php	(working copy)
@@ -558,8 +558,10 @@
 		}

 		$where = 'status = 1';
-		if ($type !== NULL)
-			$where .= (" and type='".doSlash($type)."'");
+		if( $type === 1 )	# load admin-side and library plugins...
+			$where .= ' and type in (1,2,3)';
+		else 				# load public plugins (ie: public-only and public & admin plugins)...
+			$where .= ' and type not in (2,3)';

 		$rs = safe_rows("name, code, version", "txp_plugin", $where. ' order by `order`');
 		if ($rs) {

I offer this only as a starting point for thought on the matter.

The patch…

+ Retains current behaviour for plugin loading for type 0, 1 and 2 plugins. (When called by Txp’s public and admin code — no tests run against any plugins that might call this function.)
+ Adds a new plugin type of ‘admin-only’ (value:3) as the existing ‘admin’ is really ‘admin-and-public’.
+ Doesn’t load ‘admin-only’ plugins on the public interface.
- Removes the ability to specifically target a single type of plugin.

With it, plugin writers would then have the choice of updating their admin-only plugins that currently get loaded public-side to type 3 for Txp 4.0.7. However, it would introduce the issue of backwards incompatibility of “admin-only” plugins in previous versions of Textpattern (4.0.6 and below).

Is this matter worth pursuing at all?
If so, how?

Any thoughts?

Last edited by net-carver (2008-06-20 17:14:26)

Bloke · 2008-06-20 16:24:59

I like the cut of your jib. Very useful imho.

No idea how to solve the backwards compatibility issue right now but will put it in the “processing” part of my brain to be mulled over while (probably) washing the dishes later…

ruud · 2008-06-20 17:51:23

When thinking in bits, I think this would be the ideal way to do it:

type 0 = not loaded (= lib, I presume)
type 1 = public
type 2 = admin
type 3 = admin + public (=1+2)

But that’s not very compatible with the current setup.

Steve, have you done benchmarks on how long it takes to load a set of common plugins? (compared to total page load time)

chriloi · 2008-06-20 18:05:45

Maybe this thread is not the right place to say that, but the current plugin system lacks a lot of important functionalities. For example: Priority, dependencies and more than just 2 hooks / for an event/step…

Do you see a chance for one of these things being implemented in the next version?

net-carver · 2008-06-20 18:17:37

Ruud

Sorry, I forgot to copy the runtime stats when I did the test. Hopefully I can put together more useful stats based on a defined set of admin plugins early next week.

FWIW: I came up with basically the same mapping as yourself but dropped the idea as being too incompatible with the current scheme. I do like the idea of type 0 for library plugins (and only loading them explicitly when other plugins need them.)

In the meantime though, another thought; one that you might not be able to answer publicly…

Imagine a scenario where an admin plugin (that only implements admin-side features) introduced some kind of flaw that essentially didn’t matter on the admin side as doAuth() takes place before plugins are loaded such that only valid users get access the plugin’s functions. However, this is not the case on the public side and so our hypothetical, flawed, admin plugin might introduce a publicly accessible vulnerability… perhaps?

Last edited by net-carver (2008-06-20 18:21:36)

wet · 2008-06-20 19:02:38

chriloi wrote:

For example: Priority…

Yep.

Bloke · 2008-06-20 19:11:58

chriloi wrote:

Priority, dependencies and more than just 2 hooks / for an event/step…

‘Priority’ is already in the dev bag for 4.0.7 in the form of a plugin preferred load order integer (1=load first, 9=last).
Dependency can only really be done with require_plugin() at the moment but it can’t target specific versions, afaik.
More than two hooks, hmm, not needed that myself so I can’t really comment on its usefulness.

ruud wrote:

When thinking in bits…

I agree the existing system is a tad awkward from a programming viewpoint and renumbering is attractive if it wasn’t for the incompatibility.

To that end, would it be possible to introduce a $plugin['developed_with'] into the template, or some sort of equivalent notifier that stated which version of TXP the plugin was made under? On plugin upload in 4.0.7 and above, the absence of this flag means the type needs to be converted to the new numbering scheme. In versions prior to 4.0.7. it’s ignored. On site upgrade from 4.0.6 => 4.0.7 it is assumed that all installed plugins will require their ‘type’ column altered to the new system (SVN, hmmm)

Going forward, the Plugin Composer could auto-fill this field in with the txpversion constant, so it is automatically tied to the major release used when the plugin was written. This flag also serves a dual purpose if displayed in the plugin header to show any potential uploadee which version of TXP the plugin has been developed under.

People using the zem/net-carver template manually could fill this field in themselves of course. And when new versions of TXP are released the official template should have that field updated to reflect the major release number. The only anomalies are people using SVN since the ‘new’ scheme would be in place but existing plugins would need their types changing in the interim, and when a complete upgrade to 4.0.7 comes along for these people, SVN copies > rNNNN would not need the type adjusting. Bit of a pain, but a one-time test in the upgrade script might do it?

Does something along those lines (or equivalent) work at all, or have I missed something glaringly obvious, as is my wont?

P.S. for binary comparators and useful shift operations, 0 (lib), 1 (pub), 2 (adm), 4 (pub+adm) would be my preferred numbering scheme. But I admit it doesn’t make a whole lot of sense to people without propellers in their heads.

EDIT: wet beat me to the priority thing.

Last edited by Bloke (2008-06-20 19:15:25)

jm · 2008-06-20 20:05:42

$plugin = array(
    'author' => 'John Doe',
    'author_uri' => 'http://example.com',
    'name' => 'myy_plugin',
    'plugin_uri' => 'http://example.com/plugins/myy_plugin',
    'version' => '0.1.1',
    'description' => 'Does stuff.',
    'order' => 5,
    'type' => 'admin|all|lib|public',
    'txp_version' => '4.0.7',
);

Setting type as a string would allow for backward compatibility – check if type is 0 or 1 first, then check the new types in an else.

Last edited by jm (2008-06-20 20:09:03)

ruud · 2008-06-20 21:33:33

What is the advantage of going through all that trouble instead of doing what Steve proposed?

jm · 2008-06-20 22:06:54

It was based on your post. IMO, once you validate more than just boolean, integers shouldn’t be used. Aside from being more understandable, it would be compatible with <4.0.7 (or whatever version gets the new type).

ruud · 2008-06-20 22:54:01

What I suggested in my post was how I’d do it if plugins were a new feature that could be written from scratch. Given the current situation, changing the plugin type meanings in this in the 4.0.x series is not a good idea, IMHO. Adding a type, that’s something to consider.

Plugins that take advantage of newer features in TXP are usually not backwards compatible anyway. Nothing new there. The only people who really get to see the plugin types are the plugin developers and one would assume they’re intelligent enough to understand numbers ;)

chriloi · 2008-06-20 23:02:27

Fine, that priority is in 4.0.7. Dependencies is not that important as there is an adequate workaround.

In terms of hooks, I think the more we have the simplier it is for us plugin devs to inject code or even just alter some backend functionalities.

I’m thinking of more hooks for the write tab and the article tab. Especially in the article tab, I would like to have one just after the sql, so I can filter the article list, without reproducing the whole article tab, as I had to do for one of my current plugin developments…

And to be honest, more hooks in the backend won’t be slower than the bitchy buffer overwrites or javascript injects.

Textpattern CMS

Textpattern CMS support forum

#1 2008-06-20 16:19:13