Code

Opened 6 years ago

Last modified 8 months ago

#8713 new New feature

Django core depends on django.contrib

Reported by: Piotr Lewandowski <django@…> Owned by: nobody
Component: Core (Other) Version: master
Severity: Normal Keywords:
Cc: jwilk@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I believe that it should be possible to strip django/contrib directory and still have a functional Django. However, currently there are some imports from django.contrib present in non-contrib part of Django. It's contrary to the loose-coupling idea and description of contrib package (a variety of extra, optional tools).

$ egrep -ohr --include='*.py' --exclude-dir=contrib --exclude-dir=conf 'django[.]contrib[.][[:alnum:]]+' django-trunk/django/ | sort | uniq -c | sort -n
      2 django.contrib.localflavor
      2 django.contrib.sites
      3 django.contrib.sessions
      4 django.contrib.contenttypes
      5 django.contrib.auth

$ egrep -lr --include='*.py' --exclude-dir=contrib --exclude-dir=conf 'django.contrib.[[:alnum:]]+' django-trunk/django/
django-trunk/django/test/client.py
django-trunk/django/db/models/sql/subqueries.py
django-trunk/django/db/models/fields/__init__.py
django-trunk/django/db/models/base.py
django-trunk/django/core/management/commands/cleanup.py
django-trunk/django/core/management/sql.py
django-trunk/django/core/context_processors.py
django-trunk/django/views/generic/create_update.py
django-trunk/django/views/defaults.py
django-trunk/django/middleware/cache.py

Attachments (3)

core-management-sql.diff (523 bytes) - added by Piotr Lewandowski <django@…> 6 years ago.
core-management-commands-cleanup.diff (1.7 KB) - added by Piotr Lewandowski <django@…> 6 years ago.
test-client.diff (1.4 KB) - added by Piotr Lewandowski <django@…> 6 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 6 years ago by Piotr Lewandowski <django@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Dependency on django.contrib.localflavor is already covered by #8210 and #8664.

comment:2 follow-up: Changed 6 years ago by ubernostrum

I suspect many of these are not what you think they are, or involve optional features of core functionality (e.g., generic views use auth for optional "login required" functionality).

comment:3 in reply to: ↑ 2 Changed 6 years ago by Piotr Lewandowski <django@…>

Replying to ubernostrum:

I suspect many of these are not what you think they are, or involve optional features of core functionality (e.g., generic views use auth for optional "login required" functionality).

Why generic views should even know anything about auth-related stuff? After all, we have login_required decorator in django.contrib.auth.decorators which should handle it. Now this same job is done by two distinct pieces of code - looks like DRY violation to me.

comment:4 Changed 6 years ago by Piotr Lewandowski <django@…>

Backwards compatibile issues

tests/client.py

Presence of django.contrib.session in INSTALLED_APPS is mostly checked before import. It can be done with django.contrib.auth as well, which is actually used only in Client.login() method. Or at least relevant import could be just moved to this method (ugly).

db/models/sql/subqueries.py

django.contrib.contenttypes is used in DeleteQuery.delete_batch_related() method. At least a check in INSTALLED_APPS could be done.

core/management/commands/cleanup.py

Move this command to django/contrib/session/management/commands/.

core/management/sql.py

Import is present but not used. Remove it.

views/defaults.py

Additional check for django.contrib.contenttypes and django.contrib.sites in INSTALLED_APPS.

Backwards incompatible issues

django/db/models/fields/__init__.py

Covered by #8210 and #8664.

views/generic/create_update.py

create_object() should not have login_required argument. django.contrib.auth.decorators.login_required should be used instead.

core/context_processors.py

Move django.core.context_processors.auth to django.contrib.auth.context_processsors.

No issues

db/models/base.py

django.contrib used only in a comment.

middleware/cache.py

django.contrib used only in an assertion string.

comment:5 Changed 6 years ago by Piotr Lewandowski <django@…>

  • Owner changed from nobody to pl

Changed 6 years ago by Piotr Lewandowski <django@…>

Changed 6 years ago by Piotr Lewandowski <django@…>

comment:6 Changed 6 years ago by Piotr Lewandowski <django@…>

  • Owner changed from pl to nobody

Changed 6 years ago by Piotr Lewandowski <django@…>

comment:7 Changed 6 years ago by Piotr Lewandowski <django@…>

  • Component changed from Uncategorized to Core framework

comment:8 Changed 6 years ago by adrian

  • Triage Stage changed from Unreviewed to Accepted

We've discussed this at DjangoCon (independently of this ticket) and decided it would be a good plan to refactor things so that django.contrib could be safely deleted without affecting the rest of the framework. I have not looked at the patches in this particular ticket, but I'm marking the ticket as accepted because we like the concept.

comment:9 Changed 3 years ago by jwilk

  • Cc jwilk@… added
  • Easy pickings unset
  • Severity set to Normal
  • Type set to Uncategorized

comment:10 Changed 3 years ago by ubanus@…

  • Cc ubanus@… removed

comment:11 Changed 3 years ago by lukeplant

  • Type changed from Uncategorized to New feature

comment:12 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:13 Changed 13 months ago by aaugustin

Running the command provided by the OP I still found the following uses of contrib in core.

Action needed:

  1. django/middleware/doc.py: checks request.user.is_active/is_staff
  2. django/test/client.py: depends on django.contrib.auth and has coupling with 'django.contrib.sessions'
  3. django/test/testcases.py: depends on django.contrib.staticfiles (at least)
  4. django/views/defaults.py: shortcut has a comment saying it should be deprecated in Django 2.0, but I don't see much point in waiting; we have a well established deprecation policy now and this is a trivial case.

No action needed:

  1. django/core/management/commands/cleanup.py: it's deprecated
  2. django/db/models/base.py: it's just a comment
  3. django/middleware/cache.py: tests request.user.is_authenticated() when CACHE_MIDDLEWARE_ANONYMOUS_ONLY is set. #15201 proposes to decouple CACHE_MIDDLEWARE_ANONYMOUS_ONLY from django.contrib.auth by testing only if the response varies on cookie. That covers the issue.
Last edited 8 months ago by ramiro (previous) (diff)

comment:14 Changed 13 months ago by aaugustin

comment:15 Changed 13 months ago by aaugustin

I filed #20126 for action 1.

comment:16 Changed 9 months ago by ramiro

I filed #20739 for item 3.

Last edited 9 months ago by ramiro (previous) (diff)

comment:17 Changed 8 months ago by ramiro

I filed #20915 for item 2.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


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

 
Note: See TracTickets for help on using tickets.