Code

Opened 2 years ago

Closed 2 years ago

#18018 closed Bug (invalid)

MessageFailure in Django 1.4 admin

Reported by: Simon Owned by: nobody
Component: Uncategorized Version: 1.4
Severity: Normal Keywords: admin, messages, failure
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When not having the Django messages framework installed, Django 1.4 throws as exception on saving objects via the admin interface:

MessageFailure at /admin/forum/post/5/
You cannot add messages without installing django.contrib.messages.middleware.MessageMiddleware

When I disable loc 696 in contrib/admin/options.py, everything works fine:

def message_user(self, request, message):

"""
Send a message to the user. The default implementation
posts a message using the django.contrib.messages backend.
"""
# messages.info(request, message)
pass

messages.info can only be used when the optional messages framework is installed ...?

Another minor thing:
Even with the new cookie based session backend, Django still creates a database session table. Intentionally?

Great work and thanks a lot for Django 1.4!!

Attachments (0)

Change History (14)

comment:1 Changed 2 years ago by Clueless

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

When not having the Django messages framework installed, Django 1.4 throws as exception on saving objects via the admin interface:

django.contrib.admin is documented as depending on django.contrib.messages, and django.contrib.messages.middleware.MessageMiddleware. So unfortunately those things are not optional if you want to use the admin site.

See https://docs.djangoproject.com/en/1.4/ref/contrib/admin/

comment:2 follow-up: Changed 2 years ago by akaariai

Should these dependencies be validated? Maybe part of improve error messages project?

comment:3 Changed 2 years ago by aaugustin

Another minor thing: Even with the new cookie based session backend, Django still creates a database session table. Intentionally?

This is a known trade-off, I don't believe there are any performance drawbacks to creating a single, unused table.

comment:4 in reply to: ↑ 2 Changed 2 years ago by aaugustin

Replying to akaariai:

Should these dependencies be validated? Maybe part of improve error messages project?

No, at some point we can't prevent people who don't read the first line of the docs from shooting themselves into the foot.

Last edited 2 years ago by aaugustin (previous) (diff)

comment:5 Changed 2 years ago by anonymous

Thanks for the hint. Obviously this changed over the last Django version, since up to now, we neither had the required message context processor, nor middleware or app installed. For only *one* occurrence of the messages framework inside the admin code this is actually quite some overhead. IMHO, it would make sense to make this optional and simply check availability of the framework here:

contrib/admin/options.py

messages.info(request, message)

Best,
Simon

comment:6 Changed 2 years ago by akaariai

  • Resolution invalid deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Unreviewed to Accepted

I think this is a backwards incompatible change, in 1.3 you did not need the messages framework, in 1.4 you do, and this is not mentioned in release notes. I am reopening this, please re-close if I am missing something obvious.

comment:7 Changed 2 years ago by aaugustin

Indeed - it wasn't obvious in the original report that it's a regression. It could still end up as wontfix, but with more arguments :)

comment:8 Changed 2 years ago by kmtracey

It's a planned regression -- the old messages support was deprecated and finally removed in 1.4. Thus the new way is required by admin in 1.4. I've seen other tickets go by where someone complained that final removal of something on the deprecation path wasn't noted in the release notes. This is the same issue. Answer in the past has been we don't note those in the release notes...given how many times things like this are being raised I think it might be better if we DID actually note those things that are now really gone due to finishing their deprecation cycle. People don't seem to be paying attention to the plans from previous release notes or seeing/acting on the deprecation warnings.

comment:9 Changed 2 years ago by akaariai

I still think there is problem. If you installed admin by the docs of 1.3:

Add 'django.contrib.admin' to your INSTALLED_APPS setting.
The admin has four dependencies - django.contrib.auth, django.contrib.contenttypes,
   django.contrib.messages and django.contrib.sessions. If these applications are not in your INSTALLED_APPS list, add them.
Determine which of your application’s models should be editable in the admin interface.
For each of those models, optionally create a ModelAdmin class that encapsulates the customized admin
   functionality and options for that particular model.
Instantiate an AdminSite and tell it about each of your models and ModelAdmin classes.
Hook the AdminSite instance into your URLconf.

I think you will get a non-functioning site in 1.4. 1.4 docs has this addition, and it is not mentioned in release notes.

Add django.contrib.messages.context_processors.messages to TEMPLATE_CONTEXT_PROCESSORS and MessageMiddleware to MIDDLEWARE_CLASSES.
     (These are both active by default, so you only need to do this if you’ve manually tweaked the settings.)

Of course, the proper test is to install Django + admin using 1.3 docs, upgrade to 1.4 using release notes and see if the admin still works. I haven't done that, but the above suggests there could be a problem.

Note that I am not saying that the dependency should be removed, just that the release notes should mention this.

comment:10 Changed 2 years ago by akaariai

OK, I tested upgrading from 1.3 to 1.4. In fact, things do work with the 1.3 default settings and admin installation according to docs.

The problem is if you have for some reason removed the MessageMiddleware from the default settings (or, maybe you are upgrading from 1.x -> 1.3 -> 1.4). In that case you will not get any warnings in 1.3, and in 1.4 things will not work. So, the problem is that in 1.3 MessageMiddleware was not required, in 1.4 it is and the release notes (or 1.3 warnings) do not mention this. 1.3 docs do not mention that MessageMiddleware is required. A note "make sure MessageMiddleware is in the MIDDLEWARE_CLASSES" should be in release notes. As this is a rare situation, and the error message is clear enough when you hit it, I don't think anything more needs to be done.

comment:11 Changed 2 years ago by ramiro

The deprecation timeline document says, under the 1.4 section:

[...] The Message model (in django.contrib.auth) [...] will be removed. The messages framework should be used instead. [...] Note that this means that the admin application will depend on the messages context processor.

So what we are missign is a way to effectively help users to find that document that somebody has taken to write a while back as a part of theur upgrade process.

IMHO adding notes with these kind of caveats to the release notes will render that document in a heavy piece and could introduce the risk of having manually maintained, duplicate information in two pieces of documentation.

Two different proposals:

  1. Maybe we can add prominent links pointing to the:
  • Deprecation timeline and
  • Latest release notes

(in that order) for users that are upgrading from previous versions to the Installation document (https://docs.djangoproject.com/en/1.4/intro/install/#remove-any-old-versions-of-django)?

  1. Currently, the main document index has this text in a link: "Release notes and upgrading instructions" but the target (https://docs.djangoproject.com/en/1.4/releases/) of such link is little more than a perfunctor¡y list of release notes documents, there are not actual upgrading intructions. Maybe we can add a prominent seealso or warning ReST admonition at the top of such document warning/pointing users upgrading from older version that they should read the deprecation timeline document.

Option 1 could be too heavy for a introductory installation document that is mostly read by new users. I'd prefer option 2.

Last edited 2 years ago by ramiro (previous) (diff)

comment:12 Changed 2 years ago by akaariai

I did not see that this was mentioned in the deprecation timeline. I just searched for "message" in the 1.4 release notes.

I would hope there would be a single document you need to follow to update. I am not sure, but it seems you need to read through both the deprecation time line and the release notes to upgrade, the first contains deprecations, the second backwards incompatible changes. At least adding the seealso/warning sounds like a good idea.

At this point I am not too attached to the "must add release note" idea. It is mentioned in the deprecation timeline. Install 1.3 + admin -> 1.4 works, and even if you hit the error you get a clear indication what went wrong. I am still leaving this open, but re-closing this doesn't sound like a bad idea to me.

comment:13 Changed 2 years ago by anonymous

The original error message was enough for us to see what the problem was - pretty clear. Just keep the system the way it is. I think everything works really great. I just thought it might be a bug ...

comment:14 Changed 2 years ago by akaariai

  • Resolution set to invalid
  • Status changed from reopened to closed

reclosing as invalid. Sorry for the noise.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.