Add more callback events or related variables

NicolasGraph · 2017-10-20 12:48:30

Trying to see how the upcoming skin package (by package, I mean the PHP related classes) could be used in plugins, I thought that some users could want to export theme pages, forms and styles on saving. I made that just to test the idea and the flexibility of the skin package (works with the themes branch with an existing theme directory):

# Name: oui_export v0.0.1
# Exports pages, forms and styles on saving.
# Author: Nicolas Morand
# URL: http://github.com/NicolasGraph
# Recommended load order: 5
# .....................................................................
# This is a plugin for Textpattern - http://textpattern.com/
# To install: textpattern > admin > plugins
# Paste the following text into the "Install plugin" box:
# .....................................................................

YToxMzp7czo0OiJuYW1lIjtzOjEwOiJvdWlfZXhwb3J0IjtzOjc6InZlcnNpb24iO3M6NToi
MC4wLjEiO3M6NjoiYXV0aG9yIjtzOjE0OiJOaWNvbGFzIE1vcmFuZCI7czoxMDoiYXV0aG9y
X3VyaSI7czozMDoiaHR0cDovL2dpdGh1Yi5jb20vTmljb2xhc0dyYXBoIjtzOjExOiJkZXNj
cmlwdGlvbiI7czo0MjoiRXhwb3J0cyBwYWdlcywgZm9ybXMgYW5kIHN0eWxlcyBvbiBzYXZp
bmcuIjtzOjQ6ImhlbHAiO047czo0OiJjb2RlIjtzOjEyODc6IgoKbmFtZXNwYWNlIE91aVxF
eHBvcnQgewoKICAgIGNsYXNzIE1haW4KICAgIHsKICAgICAgICBwdWJsaWMgZnVuY3Rpb24g
X19jb25zdHJ1Y3QoKQogICAgICAgIHsKICAgICAgICAgICAgLy8gQXV0byBleHBvcnRzIHRl
bXBsYXRlcyBvbiBzYXZpbmcuCiAgICAgICAgICAgIGZvcmVhY2ggKGFycmF5KCdwYWdlJywg
J2Zvcm0nLCAnY3NzJykgYXMgJHBhbmVsKSB7CiAgICAgICAgICAgICAgICByZWdpc3Rlcl9j
YWxsYmFjayhhcnJheSgkdGhpcywgJ2V4cG9ydFRlbXBsYXRlJyksICRwYW5lbC4nX3NhdmVk
Jyk7CiAgICAgICAgICAgIH0KICAgICAgICB9CgogICAgICAgIHB1YmxpYyBmdW5jdGlvbiBl
eHBvcnRUZW1wbGF0ZSgkZXZlbnQsICRzdGVwLCAkbmFtZSwgJG5ld25hbWUpCiAgICAgICAg
ewogICAgICAgICAgICBzd2l0Y2ggKCRldmVudCkgewogICAgICAgICAgICAgICAgY2FzZSAn
cGFnZV9zYXZlZCc6CiAgICAgICAgICAgICAgICAgICAgJGNsYXNzID0gJ1BhZ2VzJzsKICAg
ICAgICAgICAgICAgICAgICBicmVhazsKICAgICAgICAgICAgICAgIGNhc2UgJ2Zvcm1fc2F2
ZWQnOgogICAgICAgICAgICAgICAgICAgICRjbGFzcyA9ICdGb3Jtcyc7CiAgICAgICAgICAg
ICAgICAgICAgYnJlYWs7CiAgICAgICAgICAgICAgICBjYXNlICdjc3Nfc2F2ZWQnOgogICAg
ICAgICAgICAgICAgICAgICRjbGFzcyA9ICdTdHlsZXMnOwogICAgICAgICAgICAgICAgICAg
IGJyZWFrOwogICAgICAgICAgICB9CgogICAgICAgICAgICAkc2tpbiA9IGdldF9wcmVmKCdz
a2luX2VkaXRpbmcnKTsKCiAgICAgICAgICAgIC8vIEV4cG9ydHMgdGhlIHRlbXBsYXRlLgog
ICAgICAgICAgICBcVHhwOjpnZXQoJ1xUZXh0cGF0dGVyblxTa2luXFwnLiRjbGFzcywgJHNr
aW4sIG51bGwsICRuZXduYW1lKS0+ZXhwb3J0KGZhbHNlKTsKCiAgICAgICAgICAgIC8vIFVu
bGlua3MgYSBwb3RlbnRpYWwgb3V0ZGF0ZWQgZmlsZS4KICAgICAgICAgICAgaWYgKCRuZXdu
YW1lICYmICRuZXduYW1lICE9PSAkbmFtZSkgewogICAgICAgICAgICAgICAgJGZpbGVzID0g
XFR4cDo6Z2V0KCdcVGV4dHBhdHRlcm5cU2tpblxcJy4kY2xhc3MsICRza2luLCBudWxsLCAk
bmFtZSktPmdldFJlY0Rpckl0ZXJhdG9yKCk7CgogICAgICAgICAgICAgICAgZm9yZWFjaCAo
JGZpbGVzIGFzICRmaWxlKSB7CiAgICAgICAgICAgICAgICAgICAgdW5saW5rKCRmaWxlLT5n
ZXRQYXRobmFtZSgpKTsKICAgICAgICAgICAgICAgIH0KICAgICAgICAgICAgfQogICAgICAg
IH0KICAgIH0KCiAgICBuZXcgTWFpbigpOwp9CiI7czo0OiJ0eXBlIjtzOjE6IjQiO3M6NToi
b3JkZXIiO3M6MToiNSI7czo1OiJmbGFncyI7czoxOiIwIjtzOjg6InRleHRwYWNrIjtzOjA6
IiI7czoxNToiYWxsb3dfaHRtbF9oZWxwIjtpOjE7czozOiJtZDUiO3M6MzI6IjgwNzM1M2Nl
MDliOTBiM2RlYmM3NjYyYmIxN2MzNTZiIjt9

It works on saving but, not on duplication nor creation because we only have one callback suffixed with _saved for all these actions, with no way, unless I missed something (Oleg? Stef? :-), to figure out what is happening. In fact, it seems that I could listen the pour step for creation but I can’t find a simple way to detect a duplication.

Couldn’t we have a dedicated callback event per action, or embed an $action (or whatever appropriated name) variable to the existing event to have more infos about what the user did?

As the themes directories structure use form types as subfolders, I guess it could be also handy to get this type via the callbacks.

Last edited by NicolasGraph (2017-10-20 13:44:04)

Bloke · 2017-10-20 13:13:44

Ideally, our admin-side callbacks should have hooks for the following features:

Panel loading – broken down into various parts (header for CSS/JS injection, footer, main area with any pluggable_ui components inside).
All CRUD actions (including duplicate) for all panels. Both pre-and post-action so plugins can do something (e.g. verification) prior to an action and/or maintain their state afterwards.
Help/language system.
A way to signal an action status – e.g. success, failure. Most of our callbacks only fire if the event succeeds but it would be nice to fire a callback anyway in such a manner that plugins could detect if something didn’t go as plananed and react accordingly. This would support “rollback” in the event that a plugin raises a “pre-save” callback and does some work, then if for some reason the save fails, the failure can be detected and the pre-save stuff undone.

We mostly have (1) in place. The stuff in (2) is ongoing and being expanded but there are gaps as you found. PRs welcome to fix the holes. Hopefully (3) will start to mature when the languages-inclusion branch is rolled into core. As for (4), well, ideas welcome!

There may be other areas I’ve missed: that’s not a comprehensive list.

NicolasGraph · 2017-10-20 13:26:45

Ok, I will try to fill these gaps.
Thanks for the explanations; I will also apply this check list to the skin package.

NicolasGraph · 2017-10-26 08:31:18

About hooks, I would have few more questions…
In the Skin package, skin is a global event and I use event names such as creation, created, creation_failed.

However in other places steps are skipped and event names in use are page_saved or form_saved for example.

Could we precise the way events and steps should be used?

Here are the two options I can see:

event = panel
step = method/process(_status)

or

event = panel(_method/process)
step = status

We could also define a way to consistently name status (used as steps or step suffixes):

begin: do (the method/process name) or begin?
success: succeeded, success or done (with a method/process named do)?
failure: failed or failure?
end: end?

Last edited by NicolasGraph (2017-10-26 08:35:11)

Bloke · 2017-10-26 13:10:20

The hooks do try to conform to the event = panel / step = method/process(_status) model. At least for the panel-level hooks. Things went a little bit sideways on the Images panel because of the way the thumbnails and images are handled so the event/step got swapped over in a few places.

For the API calls, we went with event = event.step / step = status. With a few, ahem, deviations unfortunately.

This kind of thing is fine in principle:

callback_event('form.create', 'done', 0, compact('name', 'type', 'Form'));

But it causes a small issue if we want to raise one before the process so that people can do pre-form-creation preparation. Do we go with:

callback_event('form.create', 'begin', 0, compact('name', 'type', 'Form'));

or:

callback_event('form.create', 'done', 1, compact('name', 'type', 'Form'));

The latter uses the ‘pre’ modifier so that people can hook into the same event/step and differentiate on pre- or post-processing. That’s potentially neater, but the fact that the step is called ‘done’ makes it a bit clumsy. “I’d like to raise a callback on the pre-done state”. I suppose it works, but it feels a bit, I dunno… something. But I guess since it’s been introduced in many places already, we have to live with it to avoid backwards compatibility issues.

Using the former mechanism means the ‘pre’ flag is largely redundant for API calls, which seems a shame. And it introduces a new step, but that’s not so bad if we’re consistent about it.

With my hindsight hat on, perhaps:

callback_event('txp.form', 'create', 1, compact('name', 'type', 'Form'));
callback_event('txp.form', 'create', 0, compact('name', 'type', 'Form'));

would have been better? That way, the events are distinguished from ‘regular’ panel-based events of the same name (e.g. event = form / step = create), the concept of ‘step’ as ‘action’ is retained, and we get to use pre for its intended purpose.

Can we change things at this stage? Should we go through the pain now and risk backlash? Or could we offer a migration plan by permitting an array of events/steps to be supported? In other words, we could permit callback_event(_ref) to work using:

event = array('form.create', 'txp.form')
step = array('done', 'create')

And have it match the array parameters in pairs based on their position? So that form.create/done is valid, txp.form/create is valid, but form.create/create isn’t? Then we could run the two schemes in parallel for a few versions and gradually phase out the old callbacks?

NicolasGraph · 2017-10-26 13:57:03

Bloke wrote #307539:

The hooks do try to conform to the event = panel / step = method/process(_status) model. At least for the panel-level hooks.

Thus, it should be event = section + step = edit instead of event = section + step = section_edit, right?

For the API calls, we went with event = event.step / step = status. With a few, ahem, deviations unfortunately.

[…] With my hindsight hat on, perhaps:

callback_event('txp.form', 'create', 1, compact('name', 'type', 'Form'));...

I like it!

Could we offer a migration plan by permitting an array of events/steps to be supported? In other words, we could permit callback_event(_ref) to work using:

event = array('form.create', 'txp.form')...

And have it match the array parameters in pairs based on their position? So that form.create/done is valid, txp.form/create is valid, but form.create/create isn’t? Then we could run the two schemes in parallel for a few versions and gradually phase out the old callbacks?

It seems reasonable to me.
I could go with the new api related hooks in the skin package if they would be validated.

Last edited by NicolasGraph (2017-10-26 14:31:18)

Bloke · 2017-10-26 14:12:14

NicolasGraph wrote #307541:

Thus, it should be event = section + step = edit instead of event = section + step = section_edit, right?

Ideally, yes.

I could go with the new api related hooks in the skin package if they would validated.

I’m down with that. Let’s see if we have a migration path and go from there.

Textpattern CMS

Textpattern CMS support forum

#1 2017-10-20 12:48:30