Opened 9 years ago

Closed 5 years ago

Last modified 9 months ago

#26431 closed Bug (fixed)

translate_url() creates an incorrect URL when optional named groups are missing in the URL pattern

Reported by: EugeneM Owned by: Daniel Rios
Component: Core (URLs) Version: dev
Severity: Normal Keywords: translate_url
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

There is a problem when translating urls with absent 'optional' arguments
(it's seen in test case of the patch)

Attachments (2)

patch-stable-1.9.txt (2.2 KB ) - added by EugeneM 9 years ago.
patch-master.txt (2.2 KB ) - added by EugeneM 9 years ago.

Download all attachments as: .zip

Change History (13)

by EugeneM, 9 years ago

Attachment: patch-stable-1.9.txt added

by EugeneM, 9 years ago

Attachment: patch-master.txt added

comment:1 by Tim Graham, 9 years ago

Patch needs improvement: set
Summary: django.core.urlresolvers.translate_url fixtranslate_url() creates an incorrect URL when optional named groups are missing in the URL pattern
Triage Stage: UnreviewedAccepted

Could you please provide the patch as a pull request against master? Unless this is a regression, it probably doesn't qualify for a backport to 1.9 (see our supported versions policy). Please check the patch on Python 3 as well (dict.iteritems() doesn't exist there).

comment:2 by Marten Kenbeek, 9 years ago

The issue here is that resolve() will return None for any optional capture groups that aren't captured, but reverse() will use force_text() and convert it to the literal string 'None'. This causes an asymmetry between resolve() and reverse().

I think it's a better idea to treat this asymmetry as a bug, and solve it within reverse(). We would discard any arguments that are None. Incidentally this will allow for cleaner code when you don't know if an optional parameter is set when you call reverse() or the {% url %} template tag:

{% if obj.might_be_none != None %}
    {% url 'my_view' obj.might_be_none %}
{% else %}
    {% url 'my_view' %}
{% endif %}

vs.

{% url 'my_view' obj.might_be_none %}

I think that's a reasonably common usecase, so it would be nice to support this cleaner syntax. Non-existent template variables will still pass an empty string rather than None, so this won't hide any typos in your template variable names.

comment:3 by Daniel Rios, 6 years ago

Owner: changed from nobody to Daniel Rios
Status: newassigned

comment:4 by j-bernard, 5 years ago

This also affects the set_language function.
When changing language on a page with an URL pattern containing an optional group, the empty optional part is set to none and translating the page leads to a 404.
The bug on translation did not exist back in 1.8, not sure when it was introduced. I'm on 2.2.

Version 0, edited 5 years ago by j-bernard (next)

comment:5 by Mariusz Felisiak, 5 years ago

Patch needs improvement: unset
Version: 1.9master

comment:6 by Mariusz Felisiak, 5 years ago

Needs tests: set

comment:7 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In d640c71f:

Refs #26431 -- Added tests for resolving URL and translate_url() with provided optional parameter.

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 76b993a:

Fixed #26431 -- Prevented django.urls.resolve() from returning missing optional parameters.

Previous behavior was inconsistent with django.urls.reverse() and
caused that translate_url() created an incorrect URL when an optional
parameter was missing.

comment:9 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In 6cb30414:

[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

comment:10 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In 9736137:

Fixed #31069, Refs #26431 -- Doc'd RegexPattern behavior change in passing optional named groups in Django 3.0.

comment:11 by Mariusz Felisiak <felisiak.mariusz@…>, 9 months ago

In 59573829:

Refs #26431 -- Added more test for translated path().

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