[Django] #31069: Change in behavior for optional re matches
Django
------------------------------------------+------------------------
Reporter: Terence Honles | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: 3.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------+------------------------
I am mainly pointing this out as it probably should have been called out
as a breaking change from Django 2.X to 3.X (And I think the release notes
should probably be adjusted if the old functionality is not to be
restored). You can probably argue either way about if this is a bug since
it's coming from re's `match.groupdict()`. It's related to
https://code.djangoproject.com/ticket/31061 and caused by
https://code.djangoproject.com/ticket/26431 /
https://github.com/django/django/pull/11477
The test is the following:
{{{
from django.urls import re_path
def view(request, optional1, optional2): ...
urlpatterns = [
re_path(r'^(?P<optional1>\w+/)(?:(?P<optional2>\w+)/)', view)
]
}}}
For Django 2.X this would work as expected because named captures are
returned even if they are `None`, however in Django 3.0 this would raise
an error because positional parameter `optional2` is no longer provided.
(i.e. `view() missing 1 required positional argument: 'optional2'`)
You can argue that `optional2` should actually have had a default all
along, but it didn't before and code may unexpectedly break because it is
now required.
I'm not very familiar with https://code.djangoproject.com/ticket/26431,
but I think if possible it probably should have been fixed at a different
level.
--
Ticket URL: <https://code.djangoproject.com/ticket/31069>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
---------------------------------+------------------------------------
Reporter: Terence Honles | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* stage: Unreviewed => Accepted
* component: Core (URLs) => Documentation
* severity: Normal => Release blocker
Comment:
I think that the current behavior is correct, sorry for inconvenience. We
should clarified this
[https://github.com/django/django/blob/6410d38ca7255961c08f1d657fdf920555d113f3/docs/topics/http/urls.txt#L58-L61
in documentation], add release notes and a `versionchanged` annotation.
--
Ticket URL: <https://code.djangoproject.com/ticket/31069#comment:1>
Django
---------------------------------+------------------------------------
Reporter: Terence Honles | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Baptiste Mispelon):
I can't reproduce the `view() missing 1 required positional argument:
'optional2'` error with the URLconf you've provided. There aren't any
optional capture groups in your regexp so I don't see how `groupdict()`
would return `None` values.
Do you have an example URL that triggers the error?
Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/31069#comment:2>
Django
---------------------------------+------------------------------------
Reporter: Terence Honles | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jishansingh):
error is caused by missing ? at end
--
Ticket URL: <https://code.djangoproject.com/ticket/31069#comment:3>
Django
--------------------------------+------------------------------------
Reporter: Terence Honles | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* severity: Release blocker => Normal
Comment:
Issue that Terence described is that we no longer pass `None` for named
groups that are not provided. The following example
{{{
def modules(request, format):
return render(request, 'home.html')
urlpatterns = [
path('admin/', admin.site.urls),
re_path(r'^module1/(?P<format>(html|json|xml))?/?$', modules,
name='modules1'),
]
}}}
works in Django 2.2 when we call `http://localhost:8000/module1/` but
throws an error in Django 3.0:
{{{
response = wrapped_callback(request, *callback_args,
**callback_kwargs)
TypeError: modules() missing 1 required positional argument: 'format'
}}}
IMO this behavior is correct, see
[https://code.djangoproject.com/ticket/31069#comment:1 comment].
I'm decreasing a severity because it's a documentation issue and can be
backported at anytime.
--
Ticket URL: <https://code.djangoproject.com/ticket/31069#comment:4>
Django
--------------------------------+------------------------------------
Reporter: Terence Honles | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Terence Honles:
Old description:
> I am mainly pointing this out as it probably should have been called out
> as a breaking change from Django 2.X to 3.X (And I think the release
> notes should probably be adjusted if the old functionality is not to be
> restored). You can probably argue either way about if this is a bug since
> it's coming from re's `match.groupdict()`. It's related to
> https://code.djangoproject.com/ticket/31061 and caused by
> https://code.djangoproject.com/ticket/26431 /
> https://github.com/django/django/pull/11477
>
> The test is the following:
>
> {{{
> from django.urls import re_path
>
> def view(request, optional1, optional2): ...
>
> urlpatterns = [
> re_path(r'^(?P<optional1>\w+/)(?:(?P<optional2>\w+)/)', view)
> ]
> }}}
>
> For Django 2.X this would work as expected because named captures are
> returned even if they are `None`, however in Django 3.0 this would raise
> an error because positional parameter `optional2` is no longer provided.
> (i.e. `view() missing 1 required positional argument: 'optional2'`)
>
> You can argue that `optional2` should actually have had a default all
> along, but it didn't before and code may unexpectedly break because it is
> now required.
>
> I'm not very familiar with https://code.djangoproject.com/ticket/26431,
> but I think if possible it probably should have been fixed at a different
> level.
New description:
I am mainly pointing this out as it probably should have been called out
as a breaking change from Django 2.X to 3.X (And I think the release notes
should probably be adjusted if the old functionality is not to be
restored). You can probably argue either way about if this is a bug since
it's coming from re's `match.groupdict()`. It's related to
https://code.djangoproject.com/ticket/31061 and caused by
https://code.djangoproject.com/ticket/26431 /
https://github.com/django/django/pull/11477
The test is the following:
{{{
from django.urls import re_path
def view(request, optional1, optional2): ...
urlpatterns = [
re_path(r'^(?P<optional1>\w+/)(?:(?P<optional2>\w+)/)?', view)
]
}}}
For Django 2.X this would work as expected because named captures are
returned even if they are `None`, however in Django 3.0 this would raise
an error because positional parameter `optional2` is no longer provided.
(i.e. `view() missing 1 required positional argument: 'optional2'`)
You can argue that `optional2` should actually have had a default all
along, but it didn't before and code may unexpectedly break because it is
now required.
I'm not very familiar with https://code.djangoproject.com/ticket/26431,
but I think if possible it probably should have been fixed at a different
level.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/31069#comment:5>
Django
--------------------------------+------------------------------------
Reporter: Terence Honles | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Terence Honles):
Thanks @jishansingh, yes, I forgot the last `?` when typing the example
into the report.
@felixxm it would be best if this was fixed (documented) sooner rather
than later so people migrating would know they need to fix it. We didn't
have appropriate test coverage, but one of my coworkers noticed it by
chance (not a frequently used route), and checked the rest of our
application to make sure there weren't any offenders.
I don't know where to put any documentation, but I'm willing to help call
it out if I'm pointed in the right direction (I may not have time next
week so if someone else can get to it that would be great, otherwise I'll
see what I can do).
--
Ticket URL: <https://code.djangoproject.com/ticket/31069#comment:6>
Django
--------------------------------+------------------------------------
Reporter: Terence Honles | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by felixxm):
Yes, that's the right place to document this change.
--
Ticket URL: <https://code.djangoproject.com/ticket/31069#comment:7>
Django
--------------------------------+------------------------------------------
Reporter: Terence Honles | Owner: Hasan Ramezani
Type: Bug | Status: assigned
Component: Documentation | Version: 3.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Hasan Ramezani):
* status: new => assigned
* owner: nobody => Hasan Ramezani
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/31069#comment:8>
Django
--------------------------------+------------------------------------------
Reporter: Terence Honles | Owner: Hasan Ramezani
Component: Documentation | Version: 3.0
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"6cb30414bc0f83b49afc4cae76d4af5656effe9a" 6cb30414]:
{{{
#!CommitTicketReference repository=""
revision="6cb30414bc0f83b49afc4cae76d4af5656effe9a"
[3.0.x] Fixed #31069, Refs #26431 -- Doc'd RegexPattern behavior change in
passing optional named groups in Django 3.0.
Backport of 9736137cdc4b7528a0aca17415dc9798660eed81 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31069#comment:9>
Django
--------------------------------+------------------------------------------
Reporter: Terence Honles | Owner: Hasan Ramezani
Type: Bug | Status: closed
Component: Documentation | Version: 3.0
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"9736137cdc4b7528a0aca17415dc9798660eed81" 9736137]:
{{{
#!CommitTicketReference repository=""
revision="9736137cdc4b7528a0aca17415dc9798660eed81"
Fixed #31069, Refs #26431 -- Doc'd RegexPattern behavior change in passing
optional named groups in Django 3.0.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31069#comment:10>