Check new doc page for additional details

before_action/after_action would be better implemented as a plugin with an interface like chained_action so we can wrap calls to all actions with our own code.

This plugin interface would let us modify/capture parameters and return values like the code here but would also allow maintaining state across calls (setting a variable before the call and checking it after) and capturing/modifying exceptions raised by the actions.

Yes, completely agree. But here signal can be used alongside with interface. As we are using flask, i've tried to follow its ideas, and exactly the same situation described in the third paragraph of Flask docs (i've even copied it into our new documentation page):

... You will notice that there are signals that appear to do the same thing like some of the builtin decorators do (eg: request_started is very similar to before_request()). However, there are differences in how they work. The core before_request() handler, for example, is executed in a specific order and is able to abort the request early by returning a response. In contrast all signal handlers are executed in undefined order and do not modify any data.

I feel pretty strongly that before_ and after_ signals are an anti-pattern. Code that is called in a random order and relies on modifying mutable parameters passed also seems like a bad idea.

Let's add plugin interfaces that work like chained_action does for these cases instead. They are better in many ways:

• order is predictable
• values are modified explicitly by passing new values up the chain
• local variables can be used around the call to implement things like resource management or time measurement
• no special value required to indicate when not to call the original (just don't call it)
• exceptions can be captured/modified

@amercader , flask already has before/after_request, but it makes sense to re-export them from ckan.lib.signals.ckan, so that developers don't have to know implementation details and can assume that signal is completely under CKAN control.

@wardi , I agree with what you said about new interface, but you probably misread previous explanations(or, I just used wrong grammar :) )

• those before/after shouldn't be considered as two parts of a single mechanism. they are completely different events. I just can't find any better name for reflecting the moment, when those events occur.
• signals NEVER should be used for modifying mutable parameters. We have interfaces for such kind of stuff(or, in this case, we have to create a new interface). Signals are for side-effects, and one should only use them for logic, that does not depend on execution order. In this case, the good use-case for signals would be computation of total execution time for particular action during a single request(if you want to use before/after together) or saving all uncommon package_search calls with the corresponding username in order to analyze user-preferences and make some kind of suggestion based on user's behavior(yep, you can also use IPackageController.before_search for this)

Signals are not a replacement for Interfaces. In this case, I think that we can have both - interface and signal at the same time(like in the case of register_blueprint signal, that we were discussing with @amercader above). But while Interfaces are more like hooks that are used for changing behavior, signals are subscribers that are doing only additional work, not affecting original workflow.

PS I don't have any particular reasons to use those before/after_action and I'll remove them. But I want to make sure, that you see my vision of signals and don't have a feeling, like signals are "something similar to interfaces".

thanks @smotornyuk I may have misunderstood, I believed you were suggesting before_action as a good place to modify data_dict. removing before/after_action from this PR sounds good.

👍 to re-exporting flask signals so extensions have one place to register everything.

• Removed action signals
• Updated documentation. @amercader , I've added more examples and now docs should be more clear and ready for your review/improvements.

FYI the signal points we added to our site are:

• successful logins
• invalid login attempts
• password reset requests
• account creations
• calls to datastore_upsert for created and updated records
• calls to datastore_delete for removed records
• new datasets created
• datasets updated
• datasets published by an admin (marked public)
• datasets deleted
• resources added
• resources updated
• resources deleted
• errors on bulk upload to datastore (with ckanext-recombinant Excel templates)
• downloads of bulk Excel templates (ckanext-recombinant)

I look forward to this getting merged so we can add these to core or our extensions with blinker instead of the client side screen-scraping we're doing right now.

Another good candidate for a signal is whenever an activity is created. I've found many occasions when users want to export the general activity on a CKAN instance to a third party platform to perform detailed usage analysis. Having a signal that just gives you the created activity which you can then just forward to whatever tool you are using in the background would be great.

@smotornyuk I had a go at reviewing and making clearer the docs: https://gist.github.com/amercader/9c3524af1ef78bd2062cd136d954614f Feel free to add whatever you consider to this PR (I couldn't as it's not on a branch on this repo)

Do you think it's worth adding a couple more signals as part of this PR? My favourites so far from Ian's list and mine are:

• Successful login
• User created
• Activity created

Of course these can all be added later after more discussion.

Code-wise I'm happy with it. The dict returned by the interface method is a bit weird, with the keys being callables but it is well documented.

One thing I don't fully get is what is the benefit of using ISignals instead of just registering the signal with the decorator. Can you expand on that?

I also like suggested signals, so I'll try to add them to current PR during next week.

Nothing stops you from registering the signals globally, using native blinker's API(currently, we are doing so in all the local extensions, used by our clients), but in such a way, signals will always be active. This may be a problem in two cases:

1. You have installed the extension, that is listening to some signals, but you don't need to have it enabled. Instead, you want to import some class from this extension, extend it, and expose it as a feature of your own extension. For example, I often create inheritors of CKANHarvester, but the original class itself is not used, so ckan_harvester not added to enabled plugins. But, all the global subscriptions will be created as soon as you import files directly or it will be imported as a dependency of some other module. And in my case, all the theoretical subscriptions of ckan_harvester will be active even if I didn't enable it.
2. Tests. The same as for plugins - some tests must be executed without all the listeners enabled. So you need to have some switchers for listeners.

So, it's possible to use the decorator-way described in blinker documentation as long as you are sure you won't regret about it. But I would recommend avoiding this practice for general-use extensions.

@amercader , i've merged your changes.

And added the following signals:

• successful/failed login
• user created
• request/perform password reset
• datastore upsert/delete

After a quick look, I decided that it would be better to leave activity/resource/dataset signals out of this PR, because there are quite a lot of decisions about the concrete moments when they need to be sent and what exact payload should be provided.

So, if there no additional comments after newest changes, this PR is ready to go

I had a fresh look at this @smotornyuk and it still looks great. Really excited about the doors this opens

Ciao, very good work thanks for your effort.

I'm going to implement the IDomainObjectModification over my 2.8.x compatible plugin, so I can't use ISignal.
I'm doing that to notificate the deletion of a children to a parent metadata view.

IDomainObjectModification.notify_after_commit is deprecated:

have you planned to deprecate also https://docs.ckan.org/en/latest/extensions/plugin-interfaces.html#ckan.plugins.interfaces.IDomainObjectModification.notify

or it's ok to use (not having ISignal) ?

As far as i remember, notify_after_commit doesn't work in 2.8. You can either use IDomainObjectModification.notify or more low-level ISession interface. ISession is still available in v2.9 and will be migrated to events only in the next major CKAN release(which won't happen any soon and we can implement some sort of adapter from the old ISession to new events)