Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#14498 closed (fixed)

Forms passed to FormWizard.process_step are not guaranteed to have clean validate data

Reported by: Staś Małolepszy Owned by: Luke Plant
Component: contrib.formtools Version: 1.2
Severity: Keywords: regression
Cc: niels.busch@…, stas@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The docs for FormWizard.process_step at source:trunk/django/contrib/formtools/wizard.py (line 263) read:

Hook for modifying the FormWizard's internal state, given a fully validated Form object. The Form is guaranteed to have clean, valid data.

There is however a case in FormWizard._ _call_ _ when this will not be true.

In source:trunk/django/contrib/formtools/wizard.py (line 95), the forms for each previous step are instantiated but they are not validated, which makes them not have the cleaned_data property.

for i in range(current_step):
    form = self.get_form(i, request.POST)
    if not self._check_security_hash(request.POST.get("hash_%d" % i, ''), request, form):
        return self.render_hash_failure(request, i)
    self.process_step(request, form, i)

A solution would be to call form.is_valid in the above loop, or to edit the docs to account for this particular case.

Attachments (1)

14498_regression_fix.patch (5.7 KB ) - added by Staś Małolepszy 13 years ago.
A patch fixing the regression discovered in comment:5

Download all attachments as: .zip

Change History (10)

comment:1 by Luke Plant, 13 years ago

Owner: changed from nobody to Luke Plant
Status: newassigned

comment:2 by Luke Plant, 13 years ago

Triage Stage: UnreviewedAccepted

comment:3 by Luke Plant, 13 years ago

Resolution: fixed
Status: assignedclosed

(In [14290]) Fixed #14498 - Forms passed to FormWizard.process_step are not guaranteed to have cleaned_data

Thanks to stas for the report.

comment:4 by Luke Plant, 13 years ago

(In [14291]) [1.2.X] Fixed #14498 - Forms passed to FormWizard.process_step are not guaranteed to have cleaned_data

Thanks to stas for the report.

Backport of [14290] from trunk.

comment:5 by Niels Sandholt Busch, 13 years ago

Cc: niels.busch@… added
Resolution: fixed
Status: closedreopened

I believe this changeset broke the Formwizard in other ways. It is not possible to alter the form_list in process_step anymore, because the current_step form is instantiated before previous_steps processing has been done. If I change the initial step2 form to a different form when processing step1 the current_step form is clearly not the right one anymore.

Notice I am creating the formwizard in a view, so I don't have previous state cached.

in reply to:  5 comment:6 by Staś Małolepszy, 13 years ago

Cc: stas@… added

I can confirm the same issue that niels described. I have a 3-step wizard where the step1 form is set up depending on the user's choices in step0. When I go from step0 to step1 the wizard works as expected, because form = self.get_form(next_step) is executed after the processing loop from source:django/trunk/django/contrib/formtools/wizard.py#14574#L121.

However, when I go from step1 to step2, step1 gets instantiated with form = self.get_form(current_step, request.POST) in source:django/trunk/django/contrib/formtools/wizard.py#14574#L96, before the processing loop has a chance to modify the form list. As a result, step1 form is still the one I defined in my view when instantiating my wizard.

This is broken both in 1.2.X and on trunk.

by Staś Małolepszy, 13 years ago

Attachment: 14498_regression_fix.patch added

A patch fixing the regression discovered in comment:5

comment:7 by Łukasz Rekucki, 13 years ago

Has patch: set
Keywords: regression added
milestone: 1.3
Version: SVN1.2

Updated flags to current state.

comment:8 by Luke Plant, 13 years ago

milestone: 1.3
Resolution: fixed
Status: reopenedclosed

As a technicality, this is now a different bug. Since the original bug remains fixed, it is more helpful to open a new ticket - otherwise the metadata from the original bug is lost, and the change logs referring to bugs becoming confusing. I've opened the new ticket now - #15075

comment:9 by Staś Małolepszy, 13 years ago

Thanks, lukeplant, for opening #15075. I wasn't sure if I should do it or not myself. I'll know better for the next time!

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