Opened 11 years ago

Closed 11 years ago

Last modified 8 years ago

#21412 closed Bug (fixed)

Django message framework called with None

Reported by: merb Owned by: Denis Cornehl
Component: contrib.messages Version: 1.6
Severity: Normal Keywords: messages
Cc: c.schmitt@… Triage Stage: Ready for checkin
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Currently, this is a very small bug and i don't now the first occurance, since i first used the message framework at the last evening.

I used Django 1.6 and installed everything. I put the Message Framework into the middleware, the context preprocessor and the INSTALLED APPS.
And then I did something which happend, cause i was very sleepy.

I made a generic.FormView like this:

class CreateUserView(generic.FormView):
    template_name = 'envisia_backup/create_user.html'
    form_class = UserForm

    def get_success_url(self):
        return reverse('envisia:index')

    def form_valid(self, form):
        data = form.cleaned_data
        user = User.objects.create(**data)
        user.is_staff = True
        user.is_superuser = True
        user.save()
        messages.success(request, 'User erfolgreich erstellt!')
        return HttpResponseRedirect(self.get_success_url())

Since I was sleepy I really didn't know where the mistake was.
The error message always was that the message framework wasn't installed and I should put it in the middleware and in the INSTALLED_APPS.

Why did this happen? Currently it happend cause I called messages.success with None.
request wasn't defined. so it was None. It took a while since I found the error since I never got a correct error message.

Currently the fix would be really simple. But I don't want to make a patch until know since I don't know if it is wished that the messages framework should make a assert if we call it with a Request object or not.
If it is wished that the message framework could only be called with a request object, than i would make a patch with a corresponding test. But only if this is really wished.

Change History (11)

comment:1 by Baptiste Mispelon, 11 years ago

Triage Stage: UnreviewedAccepted

Hi,

The code you showed would actually raise a NameError (it's the error python raises when you use a variable without defining it first).

The code is wrong but the behavior you describe is actually correct: passing anything but a request object (actually, anything without a _messages attribute) to add_message raises an error telling you to install the message middleware.

I can see this being confusing, so if you have ideas on how to improve the situation, please do share them.

Thanks.

comment:2 by merb, 11 years ago

Thats correct, normally it would raise NameError, but it didn't raised the NameError so it always raised that the middleware was not installed.
I was very sleepy thats why i found out, that something is really wrong.

I would make assertion checks on

messages.add_message
messages.debug
messages.info
messages.success
messages.warning
messages.error

but that would raise another problem, what happens if somebody makes a custom Request that inerhits from the actual request object, would this work with a assertion check, too?

comment:3 by Claude Paroz, 11 years ago

If this didn't raise NameError, it is probably because request was defined somewhere in your file. If we can find a rather common possible use case where request is None, then it might be worth asserting request is not None in add_message.

comment:4 by Denis Cornehl, 11 years ago

will work on a patch for this

comment:5 by Denis Cornehl, 11 years ago

Owner: changed from nobody to Denis Cornehl
Status: newassigned

comment:6 by Baptiste Mispelon, 11 years ago

Triage Stage: AcceptedReady for checkin

Looks good. I'll make sure the test suite passes and commit it.

comment:7 by Baptiste Mispelon, 11 years ago

Resolution: fixed
Status: assignedclosed

comment:8 by Raffaele Salmaso, 8 years ago

Hi,
I'm using django-rest-framework Request object, and this patch make impossible to use it as replacement: I must use request._request, which is a bit ugly.

Would it be better as

def add_message(request, level, message, extra_tags='', fail_silently=False):
    """
    Attempts to add a message to the request using the 'messages' app.
    """
    if request and hasattr(request, '_messages'):
        return request._messages.add(level, message, extra_tags)

    if not fail_silently:
        from django.conf import settings
        if 'django.contrib.messages.middleware.MessageMiddleware' not in settings.MIDDLEWARE:
            raise MessageFailure(
                'You cannot add messages without installing '
                'django.contrib.messages.middleware.MessageMiddleware'
            )
    raise TypeError("add_message() argument must be an HttpRequest like object, "
                    "not '%s'." % request.__class__.__name__)

?

comment:9 by Tim Graham, 8 years ago

Perhaps. Generally we prefer ducktyping over isinstance.

comment:10 by Chris Portela, 8 years ago

I am also using Django Rest Framework and came here to second Raffaele's proposal here.

comment:11 by Tim Graham, 8 years ago

Please open a new ticket rather than commenting on a closed one. Thanks.

Note: See TracTickets for help on using tickets.
Back to Top