Feature idea: forms signals

[David Seddon]

Hi all,

One thing I've always found challenging with Django is to adjust the functionality of forms from other apps.  An example might be to add an extra field to a login form provided by a third party module.

What I end up doing is often overriding the urlconf, view and form class.  Or, worse, monkey patching the form class.

A much nicer way to do this would be to add a few well chosen signals to forms.

Potential ones could be:

- post_init

- post_clean

- post_save (for ModelForms)

I'd be happy to do a pull request for this, but just wondered what people think.

David

[Aymeric Augustin]

Hello,

My experience with Django — and not just with inexperienced developers — is that signals are a footgun. I've seen them misused as workarounds for badly structured code and unclear dependency trees too often. I'm not eager to encourage their use.

Fundamentally, signals in Django allow a developer to declare a function call at the callee site instead of at the caller site. That provides a way for app developers to modify the behavior of libraries written by others, but not a very good one.

"Signals as an API" have the drawbacks as "subclassing as an API", such as making it difficult to tell what happens just by following function calls in the code, and then some, for example :

- managing exceptions: while signals give control back from the library to the app, they also create their own tiny area of inversion control where the signals framework manages execution of signal receivers ; you can either get all exceptions or ignore them all, but it's the library that makes this decision, not the app.

- transactional behavior: as a consequence of the previous point, it's very hard to make guarantees about what the consistency of code that executed successfully or failed to execute when signals are involved.

- execution order of receivers: I'm supposed to have rewritten app-loading in Django 1.7 to make it deterministic, but I still don't quite believe it's reasonable to rely on import order.

- no encapsulation: even if encapsulation is merely a convention in Python, at least inheritance defines some boundaries on the behavior.

I'd really prefer if Django encouraged people to support customizing forms through dependency injection, which in my opinion is the proper way to perform such customizations. It can be as simple as specifying a dotted Python path to a form class in settings if there's a desire to stay away from the factory pattern and minimise boilerplate setup code.

I must be one of the more anti-signals people here — I could make the argument that signals aren't GOTOs, they're COMEFROMs with a straight face — so don't take may opinion as reflecting the consensus. It would be nice to know what other solutions you considered before proposing this one, though.

Best regards,

-- 

Aymeric.

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/d936a1d5-3f6e-4190-83c3-5cc31c8fbb4e%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Brice PARENT]

Hi !
I'd prefer to that a standardized way of overloading classes, like with
a proper setting :
OVERLOADED_CLASSES = {
'app.forms.MyForm': 'myapp.forms.MyForm'
}
Having (or not) myapp.forms.MyForm extend app.forms.MyForm. So whenever
a CBV declares its form is app.forms.MyForm, it would instead create an
instance of myapp.forms.MyForm. Same thing for the instanciation of the
views classes.
After thatn it gets a bit trickier for the manual instanciation you
could find in FormView.get_form() for example. But I'm not fully aware
of Python's possibilities regarding such a functionality. As the mocking
library allows to replace a class with another on the fly, I'm pretty
sure it can be done, but I have no idea at what expense that would be
and what the side effects could be.

Anyway, I fully agree that there's something to do about it, as the ways
to do it by now are not really obvious.

Brice

[David Seddon]

I totally agree that signals are open to misuse, and I take your points.  Inexperienced developers often seem to use them just because they're there, rather than to encapsulate their apps and avoid circular dependencies.  The Django signals documentation could probably do with a bit more guidance of when to use them.  On the other hand, I have seen a lot of great uses of models signals too.

There are other solutions, and I've actually given a talk about these here (https://skillsmatter.com/skillscasts/7129-encapsulated-django-keeping-your-apps-small-focused-and-free-of-circular-dependencies).  The idea of OVERLOADED_CLASSES above is also a really interesting idea, but a lot more of a fundamental shift than just dispatching a few signals from the existing form classes.

What I would say in terms of form signals support is that in a former life I was a Drupal developer.  That framework has shortcomings but there are also things that work really well, and one of the best is a hook for altering forms (hook_form_alter) which works very well for Drupal's highly modular approach.  This hook is used everywhere in Drupal projects.  I think exposing this kind of hook would make it a lot easier for Django developers to architect their projects as small, pluggable apps - something that is pretty difficult to make work with forms.

David

[Aymeric Augustin]

Hi David,

It's good to hear that you did significant research in this area (which I couldn't tell from your original email).

I get how this feature can come in handy and I won't stand in the way if it gets implemented.

Improving the guidance about when (not) to use signals in the process would be great.

-- 

Aymeric.

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/56eb4ce1-b0d5-443c-bd6a-c230fc41c6b5%40googlegroups.com.

[James Pic]

Seems similar to this discussion: https://groups.google.com/forum/#!searchin/django-developers/forms%7Csort:relevance/django-developers/zG-JvS_opi4/wS-4PBfPBwAJ

Yes, signal/slot is a pattern that allows quick and easy decoupling of components that have to work on the same payload, and I've been using happily for at least a decade but I take your point as well.

About OVERLOADED_CLASSES, I don't think it's flexible enough, ie. to allow an AppConfig to override a field of another app's form class, that was made possible by David's initial proposal using signals.

If we're going to change this bit in Django, I'd like to recommend that we try to cover a broader spectrum of use case, than just allowing a user not to override a url to pass a different form.

[Adam Johnson]

I'm +1 on adding some signals to forms, because I feel it's weird for Django to have several signals for models, and none for forms. However I don't really have an opinion on what the useful signal points are, because I don't touch forms that often.

--

You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.

To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscribe@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.

Visit this group at https://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAC6Op18ifztiCqCm1Vr39%2BNGJJpCnu8a3gmYZ8sBMXyGtFZwSg%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--

Adam

[Melvyn Sopacua]

On Monday 06 March 2017 10:10:41 David Seddon wrote:

> Hi all,

>

> One thing I've always found challenging with Django is to adjust the

> functionality of forms from other apps. An example might be to add an

> extra field to a login form provided by a third party module.

>

> What I end up doing is often overriding the urlconf, view and form

> class. Or, worse, monkey patching the form class.

>

> A much nicer way to do this would be to add a few well chosen signals

> to forms.

>

> Potential ones could be:

>

> - post_init

 

Putting in a +1 and use case for pre_init: Inject dynamically generated form.prefix.

 

Right now I have to inject PrefixedFormView in every view using it and carefully watch my mro of already complex views.

 

Use case is having several modals containing forms on a single page ("Edit this", "Add related", "New this"). They need to be prefixed to deal with naming conflicts since id_{fieldname} will not be page unique anymore.

 

With the signal, I can do away with the view mixin and the WrappedForm is solely responsible for it.

I have no preference for the technology used (signal, hook, injection, new buzzword), as long as the net result is that the object creating the prefix can handle the prefix for all views.

 

Is there a ticket yet?

 

Use case code:

 

class PrefixedFormView(generic.FormView):
def post(self, request, *args, **kwargs):
self.prefix = request.GET.get('prefix', None)
return super().post(request, *args, **kwargs)

 

class WrappedForm(object):
def __init__(self, form_class: BaseForm, action_url: str,
is_multipart: bool = False, model_instance: Model = None,
initial: dict = None, prefix: str = 'auto', **kwargs):

<snipped for brevity>
if prefix == 'auto':
self.prefix = uuid4().hex
else:
self.prefix = prefix
self.form = None
self._set_query_prefix()

def _set_query_prefix(self):
final_url = urlparse(self.action_url)
query = QueryDict(query_string=final_url.query, mutable=True)
query['prefix'] = self.prefix
parts = (final_url.scheme, final_url.netloc, final_url.path,
final_url.params, query.urlencode(), final_url.fragment)
self.action_url = urlunparse(parts)

 

--

Melvyn Sopacua

[Melvyn Sopacua]

Apologies for the formatting.

https://gist.github.com/melvyn-sopacua/dbf3c8f71023d6fc261131cb0a851f58

 

 

--

Melvyn Sopacua

[David Seddon]

I've created a ticket for this here: https://code.djangoproject.com/ticket/27923#ticket

What are the next steps?  Shall I create a pull request, or does this need more consensus first?

David

[Florian Apolloner]

On Friday, March 10, 2017 at 10:12:54 AM UTC+1, David Seddon wrote:

What are the next steps?  Shall I create a pull request, or does this need more consensus first?

Imo absolutely more consensus, especially (to minimize the work for you), please do not just suggest the signals but also their signatures and also outline usecases for each of them. Once you outline the signatures it will be easier for others to understand what you can do with them etc…

[Aymeric Augustin]

Hello,

I suspect there hasn't been much feedback because people have been used to doing without form signals. My own reaction was more a knee-jerk about signals than anything else.

The justification you offered made sense, but was terse. If you had the time to write down the use cases and to justify the design of the API in a DEP, that could help a lot (but I'm not saying this is a prerequisite, for fear of sounding like I'm resorting to bureaucracy to block changes I'm not fully comfortable with).

Best regards,

-- 

Aymeric.

[David Seddon]

I've put together a brief proof of concept here:

https://github.com/seddonym/formsignals

It demonstrates what you could do with post_init, post_clean and post_save signals, which I've also implemented in a fork of Django (no test coverage or documentation yet though).

The concept it demonstrates is two apps, a blog app and a priority app.  The priority app is a pluggable app, unknown to the blog app, that allows users to mark blog entries as 'priority', providing they also put the word "Priority" in the title.  (Bizarre example but I wanted to demonstrate why you might want to use post_clean.)

The main place to look is in the receivers file here: https://github.com/seddonym/formsignals/blob/master/formsignals/priority/receivers.py

I don't think this is a nice as subclassing the form in the first place, but that's not an option unless the maintainer of the blog app has exposed a way of hooking into it.  As you can see you get a fair amount of power to encapsulate different logic between apps with very little code.

I think the use cases for pre_init, pre_clean and pre_save are more niche, but it probably makes sense to include them for completeness.

Would value any comments!

[jp...@yourlabs.org]

Hello David,

Is it possible to try to use it as part of the Django fork you mention, instead of the app ?

I couldn't find any link on your github profile, your website and google but perhaps I've missed it.

Thanks !

Best

<3

[David Seddon]

Hi JP,

That's a good point, sorry I didn't make it clear.

The fork is installed from within the requirements file but you can also see it here, on the ticket_27923 branch: https://github.com/seddonym/django/tree/ticket_27923

I've updated the README to help.

I'd be interested to hear if anyone thinks it's worth me making a pull request for this.

David