Plugin filtering for admin list pages

mrdale · 2012-07-26 00:22:20

Yay, I think{?}

etc · 2012-07-26 07:11:24

Yay+1!

Bloke wrote:

I could stuff this in now on all ‘list’ pages…

Me feeling is that it could be implemented more widely, probably by introducing callback argument in safe_rows_like functions, why not?

Bloke · 2012-07-26 08:48:00

etc wrote:

Me feeling is that it could be implemented more widely, probably by introducing callback argument in safe_rows_like functions, why not?

Do we always know the $event and $step in safe_* functions? I’m not convinced we do. If not we can’t very easily distinguish between one callback and another.

etc · 2012-07-26 11:30:54

Bloke wrote:

Do we always know the $event and $step in safe_* functions?

Not internally, but we can pass them to safe_* as appropriate. For example, we could call safe_count('textpattern', "$criteria", $debug, 'list.criteria'); in list_list(), so safe_count() would know that it has to fire $where = callback_event('list', 'criteria') . $where; before querying the db.

Last edited by etc (2012-07-26 11:31:51)

Bloke · 2012-07-26 11:49:45

etc wrote:

Not internally, but we can pass them to safe_* as appropriate.

OK. So this possible extra param that we split at . inside safe_* functions:

What impact on performance would it have to detect the presence of the 5th param and issue the callback at this level?
I assume we would only fire the callback if the event/step param was present?
If so, would this mitigate any potential attack vectors from the public side, given that safe_* are used extensively in, say, taghandlers and commenting? Could someone potentially inject/fake the 5th param (somehow?) and trigger the callback from the public side?

It seems nice to do it at the DB level as it opens up all sorts of possibilities but I’d need to be sure it’s controllable, safe and still fast before going this route. Plus it may mean waiting for the next release because it’s a broader change than just adding callbacks to the list pages directly.

etc · 2012-07-26 12:57:41

Very small per se, but given that many functions (like list_list) do multiple calls to safe_*, each registered plugin will be called as many times, so this is a real issue.
Yes, certainly, unless the performance impact is so low that it is worth doing systematically. event, step and so on could be retrieved from global variables.
We could introduce safe_*_with_callback functions on the admin side to mitigate this hypothetical risk (I have no idea how a client could fake a param).

But you have convinced me, I am not convinced anymore in its utility. May be we should wait to see if some usecases arise, and certainly until some post 4.5 release.

Bloke · 2012-07-26 13:40:34

etc wrote:

event, step and so on could be retrieved from global variables.

^{/me puts on his nervous trousers}

I have no idea how a client could fake a param

Nor do I! Was just throwing it out there in case it was a distant possibility via some PHP / global / cookie / injection wrangling.

I could certainly put this in place now on the list pages — i.e. not at the safe_* level. Worst (or best?) case scenario is that the calls move to the safe_* level in some future release; at that point, from an outside plugin perspective, very little would change in real terms as the callback event/step would stay the same for any given Page. Plugin callbacks would just be called more often per page, with the same event/step, so the output from the callback would remain the same in each safe_* call.

Last edited by Bloke (2012-07-26 13:41:26)

etc · 2012-07-26 16:28:20

Bloke wrote:

^{/me puts on his nervous trousers}

Man! I use these global $prefs on nearly everyday basis, and no one ever alerted me I needed a special equipment?

List pages are fine, great if you can do it right now. But I meant this could be useful in some other places, where a regular callback or pluggable_ui is an overkill. If someone needs to restrict the Section list on the Write tab according to the author privs, for example. But safe_* level is finally not a good idea for the moment, because of the callback redundancy, you are right (and I am wrong).

Gocom · 2012-07-26 17:13:43

I wouldn’t put the callback event to the DB functions. It seems the wrong place to me. I would add it to the corresponding locations where list filtering is needed. Application and its callbacks have no place in the DB abstraction layer.

In any case, adding the callback event to safe function doesn’t even shorten the code significantly. No new possibilities are introduced, since the option to use callbacks_events in those contexts is always there nonetheless what DB wrapper does. The panel specific code still needs alterations where the callback parameter is supplied.

As far as performance and security goes, this would have no effect to either. An extra parameter and boolean operator don’t do nor use shit, and security hazards — no.

Bloke · 2012-07-26 19:25:07

OK, thanks for the input guys. One more question: what’s the best name for the event? As mentioned if the event is simply set up like this:

$criteria .= callback_event('image', 'criteria', 0, $criteria);

then any plugin that hooks into that panel’s event (alone) will run when the ‘criteria’ step is hit. That’s pretty much a guarantee that it’ll break the output. If we could rely on there being a fixed step then it wouldn’t be so bad, but step=list is hardly ever used, and there are other steps like section_create, and so on which means authors (quite rightly) just hook into the event and do step filtering in their code.

Two ways I can see to go:

As above. Plugins will break the panels and will need amending to detect the ‘criteria’ step and ignore it if they don’t require it.
Change the event to something like ‘list_filter’ or ‘list_criteria’. In that case, one could argue that the step is redundant so the call could become:

$criteria .= callback_event('image_criteria', '', 0, $criteria);

although for completeness it’d probably be a good idea to keep it so the step can be targeted uniquely in case other *_criteria events are introduced in future.

Option 1 is cleaner, retaining a more consistent naming convention but with admin-side breakage. Option 2 breaks tradition but is less work for everyone to get things running under 4.5.0.

I favour option 2 because I’m lazy, but my OCD monkey says Option 1 is more ‘correct’.

Thoughts? Any other options I’ve neglected to think through? Any preference on naming convention?

Last edited by Bloke (2012-07-26 19:30:25)

Gocom · 2012-07-26 20:06:51

In my opinion the option 1 seems close to invalid, and less cleaner than the second one. The event should be an unique in this case as with other distinct callbacks that can’t be shared. For instance this callback can’t accept any other output than SQL.

These unique compound event names are already used elsewhere, so I don’t see any problem using them. Public facing callbacks in general only use events, separating the event and step with a full stop. On admin-side, you can find underscores used as separators, e.g. article_saved and article_posted.

To me best option could be to make the event unique to the panel, while offering the step to target just specific criteria. This in the case where a panel has multiple lists or instances that need same filtering. For example for multi-edit’s assigns operations which would then need same filters as the list.

For instance:

callback_event('image_criteria', 'list');

The $criteria parameter doesn’t actually have any need, does it? It’s not like you can use it for anything or alter it.

When dealing with event names, just make sure that the name is unique and that there can not be collisions in the future. For instance, if criteria is used as the event name, then no other even can really end with term criteria. Which is why prefixing could be better option than suffixing. E.g.

callback_event('sql.criteria_{event}', '{instance}');

Or maybe the even could be shared by whole admin-side as it’s with admin_* and pluggable_ui. E.g.

callback_event('admin_criteria', 'image_list');

Last edited by Gocom (2012-07-26 20:10:13)

Bloke · 2012-07-26 20:14:32

Gocom wrote:

callback_event(‘admin_criteria’, ‘image_list’);

Me likeee. Me adoptee.

The $criteria parameter doesn’t actually have any need, does it?

Maybe not. I was going to include it in case a plugin wanted to check the current criteria string in use so it could ‘bail out’ if perhaps another plugin had done something similar already. Long shot, but, well, umm, dunno. Is the plugin stack even cumulative or does each plugin get the “vanilla” value? Hmmm. I’ve only ever tried it with pluggable_ui() and found it not to be cumulative.

Last edited by Bloke (2012-07-26 20:17:22)

Textpattern CMS

Textpattern CMS support forum

#13 2012-07-26 00:22:20

Re: Plugin filtering for admin list pages

#14 2012-07-26 07:11:24

Re: Plugin filtering for admin list pages

#15 2012-07-26 08:48:00

Re: Plugin filtering for admin list pages

#16 2012-07-26 11:30:54

Re: Plugin filtering for admin list pages

#17 2012-07-26 11:49:45

Re: Plugin filtering for admin list pages

#18 2012-07-26 12:57:41

Re: Plugin filtering for admin list pages

#19 2012-07-26 13:40:34

Re: Plugin filtering for admin list pages

#20 2012-07-26 16:28:20

Re: Plugin filtering for admin list pages

#21 2012-07-26 17:13:43

Re: Plugin filtering for admin list pages

#22 2012-07-26 19:25:07

Re: Plugin filtering for admin list pages

#23 2012-07-26 20:06:51

Re: Plugin filtering for admin list pages

#24 2012-07-26 20:14:32

Re: Plugin filtering for admin list pages

Board footer