Upgrading from 1.6.1 to 1.7.* - forms don't work

[uri-rodberg]

• Package version: 1.7.2 (or 1.7.0 or 1.7.1)
• Django version: 1.11.16
• Python version: 3.5.2
• Template pack: (Optional)

Description:

• We upgraded Speedy Net from django-crispy-forms==1.6.1 to django-crispy-forms==1.7.2

• We didn't expect anything to change.

• Our contact form stopped working in all 4 websites.

Preferably also include:

• Example Django Crispy Forms code

The tag is {% crispy form %} on feedback_form.html

• Screenshots

• With django-crispy-forms==1.6.1:
  

• With django-crispy-forms==1.7.2:

• Actual HTML generated

There is no <textarea ... in the HTML with 1.7.0 up to 1.7.2.

• Expected HTML

There is <textarea ... in the HTML with 1.6.1.

I can't paste HTML into this issue, I tried.

[uri-rodberg]

I checked and it also doesn't work properly with versions 1.7.0 and 1.7.1.

[carltongibson]

Hi @uri-rodberg. Curious one. Are you able to bisect the issue to a particular change? (It's not initially obvious what might be the issue...)

[mostafagafer]

I having the same issue , and I asked it on https://stackoverflow.com/questions/54101476/crispy-forms-and-bootstrap-styles?noredirect=1#comment95058861_54101476 and I hope any one could help me with it

[uri-rodberg]

I having the same issue , and I asked it on https://stackoverflow.com/questions/54101476/crispy-forms-and-bootstrap-styles?noredirect=1#comment95058861_54101476 and I hope any one could help me with it

Which versions of django-crispy-forms did you try using and did it work with any of them?

[uri-rodberg]

Hi @uri-rodberg. Curious one. Are you able to bisect the issue to a particular change? (It's not initially obvious what might be the issue...)

It's a change in django-crispy-forms and not in our code. Our same code works with django-crispy-forms==1.6.1 and doesn't work with django-crispy-forms==1.7.0 to 1.7.2. But if you're asking about a particular change in django-crispy-forms, I don't know. I noticed it's a year difference between the releases of 1.6.1 and 1.7.0 (which is already more than a year old). You probably had many changes in this year. I think at least 100 commits.

[carltongibson]

If you can put it in a test case we (you?) can use git bisect to find the commit that introduced the regression.

Very happy to look at it, but need a failing example...

[mostafagafer]

I having the same issue , and I asked it on https://stackoverflow.com/questions/54101476/crispy-forms-and-bootstrap-styles?noredirect=1#comment95058861_54101476 and I hope any one could help me with it

Which versions of django-crispy-forms did you try using and did it work with any of them?

I've just instaleed it today, by pip install --upgrade django-crispy-forms
i belive it is 1.7.2

[uri-rodberg]

I created a test that checks it - you can see it on https://github.com/speedy-net/speedy-net/blob/staging/speedy/core/feedback/tests/test_views.py. If you run tests with django-crispy-forms==1.6.1 all the feedback tests pass. If you run tests with django-crispy-forms==1.7.2, 6 feedback tests fail on the same line - lines 44 and 45 on this file will fail. But lines 87 and 88 don't fail - if the user is logged in then the form is OK. I don't know why.

Compare https://travis-ci.org/speedy-net/speedy-net/builds/477445100 and https://travis-ci.org/speedy-net/speedy-net/builds/477451490
(Tests which are not related to feedback fail for other reasons and it's not related to django-crispy-forms)

You can run tests with python manage.py test speedy.core.feedback to run only the feedback tests.

You can compare https://travis-ci.org/speedy-net/speedy-net/builds/477460948 and https://travis-ci.org/speedy-net/speedy-net/builds/477460938.

@carltongibson Did I answer your question?

[uri-rodberg]

I having the same issue , and I asked it on https://stackoverflow.com/questions/54101476/crispy-forms-and-bootstrap-styles?noredirect=1#comment95058861_54101476 and I hope any one could help me with it

Which versions of django-crispy-forms did you try using and did it work with any of them?

I've just instaleed it today, by pip install --upgrade django-crispy-forms
i belive it is 1.7.2

Check with 1.6.1 and let me know if it works. I'm not sure it's related to the same issue I posted.

[mostafagafer]

it gives me error in th cmd :

django.template.library.InvalidTemplateLibrary: Invalid template library specifi
ed. ImportError raised when trying to load 'crispy_forms.templatetags.crispy_for
ms_tags': No module named 'django.core.urlresolvers'
[09/Jan/2019 19:59:53] "GET / HTTP/1.1" 500 168519

[mostafagafer]

and if I imported :
from crispy_forms.helper import FormHelper

in the forms.py it will yield another error :

File "D:\installed_apps\python\lib\site-packages\crispy_forms\helper.py", line
4, in
from django.core.urlresolvers import reverse, NoReverseMatch
ModuleNotFoundError: No module named 'django.core.urlresolvers'

[uri-rodberg]

@mostafagafer Why do you think it's related to this issue?

[carltongibson]

@uri-rodberg. Can you clone the repo then pip install -e . into (a copy of) your project virtualenv.

Checkout a failing commit then run git bisect to identify where the change was introduced.

Here's the top google hit for "Django git bisect". It looks good: https://alexlouden.com/posts/2016-git-bisect-django-unit-tests/

[uri-rodberg]

@carltongibson I will try.

By the way, I see I upgraded django-crispy-forms to 1.7.2 six months ago (on June 27 - speedy-net/speedy-net@91a0f9f) but tests didn't fail so I was not aware of this bug. I added the tests today which fail when this bug exists.

[carltongibson]

@mostafagafer The line causing your error was changed in #681 and was part of v1.7. Your issue is unrelated to the current topic. I advise you run pip freeze to check the installed version. (You need Django 1.11 or lower to use Crispy forms 1.6.x.)

[uri-rodberg]

@uri-rodberg. Can you clone the repo then pip install -e . into (a copy of) your project virtualenv.

Checkout a failing commit then run git bisect to identify where the change was introduced.

Here's the top google hit for "Django git bisect". It looks good: https://alexlouden.com/posts/2016-git-bisect-django-unit-tests/

pip install -e . Doesn't work for me, I get an error message. Where is it documented?

[carltongibson]

http://pip.pypa.io/en/stable/reference/pip_install/ see “Editable Installs”.

You’ll need to have your virtualenv activated. Then the . is the path to your Crispy Forms clone. (It’s the folder including cf’s setup.py.

If you paste the error I may be able to assist. (But it should just install)

[uri-rodberg]

http://pip.pypa.io/en/stable/reference/pip_install/ see “Editable Installs”.

You’ll need to have your virtualenv activated. Then the . is the path to your Crispy Forms clone. (It’s the folder including cf’s setup.py.

If you paste the error I may be able to assist. (But it should just install)

OK, I didn't understand you want me to clone your repository.

I did it and got the following output:

ImportError: cannot import name 'memoize'
6b93e8a is the first bad commit
commit 6b93e8a
Author: Greg Hinch greg@greghinch.com
Date: Tue Jan 20 10:22:32 2015 +0000

We should still respect the configuration of the Helper object when rendering Meta fields

:040000 040000 5dc0ff513afbce82db9e0ef5573b94b5de1ce6c5 d046dfde517fcadf347cf9821c6a15923a404411 M crispy_forms
bisect run success

But I'm not sure how it is related to a commit from 2015.

[uri-rodberg]

OK, I think there is a bug with git bisect. I ran this manually and found these results:

Commit 23e4c63 - everything is working.

Commit 20bf63b - not working.

I think all commits after 20bf63b are not working, and all commits before 23e4c63 are working.

I hope this helps.

[uri-rodberg]

I found out what I can change to make this work:

$ git diff master
diff --git a/crispy_forms/helper.py b/crispy_forms/helper.py
index 9b8aa43..a9d0970 100644
--- a/crispy_forms/helper.py
+++ b/crispy_forms/helper.py
@@ -313,11 +313,7 @@ class FormHelper(DynamicLayoutHandler):
             fields = set(form.fields.keys())
             left_fields_to_render = fields - form.rendered_fields
             for field in left_fields_to_render:
-                if (
-                    self.render_unmentioned_fields or
-                    self.render_hidden_fields and form.fields[field].widget.is_hidden or
-                    self.render_required_fields and form.fields[field].widget.is_required
-                ):
+                if (True):
                     html += render_field(
                         field,
                         form,
@@ -339,13 +335,7 @@ class FormHelper(DynamicLayoutHandler):
                 for field in left_fields_to_render:
                     # We still respect the configuration of the helper
                     # regarding which fields to render
-                    if (
-                        self.render_unmentioned_fields or
-                        (self.render_hidden_fields and
-                         form.fields[field].widget.is_hidden) or
-                        (self.render_required_fields and
-                         form.fields[field].widget.is_required)
-                    ):
+                    if (True):
                         html += render_field(field, form, self.form_style, context)

         return mark_safe(html)
warning: LF will be replaced by CRLF in crispy_forms/helper.py.
The file will have its original line endings in your working directory.

If I change the 2 ifs to True then it works.

Did I miss anything in the settings?

[uri-rodberg]

I checked your code and the following if (lines 342 to 348 in crispy_forms/helper.py):

                    if (
                        self.render_unmentioned_fields or
                        (self.render_hidden_fields and
                         form.fields[field].widget.is_hidden) or
                        (self.render_required_fields and
                         form.fields[field].widget.is_required)
                    ):

It looks to me that it's not necessary since the fields in class Meta should always be rendered.

You can see our code on https://github.com/speedy-net/speedy-net/blob/staging/speedy/core/feedback/forms.py and the fields defined in class Meta are ('sender_name', 'sender_email', 'text'). But, notice in one case we delete the first 2 fields without changing class Meta.

[carltongibson]

... if the user is logged in then the form is OK. I don't know why.

So what's different here? What about logging-in changes how the conditional evaluates? How is that something to do with Crispy Forms? (It may be that it is, but I'm not quite seeing it yet...)

[uri-rodberg]

So what's different here? What about logging-in changes how the conditional evaluates? How is that something to do with Crispy Forms? (It may be that it is, but I'm not quite seeing it yet...)

@carltongibson I don't understand, what do you want me to do? Is it a bug in django-crispy-forms or is it the expected behavior? I downgraded Speedy Net to django-crispy-forms==1.6.1 because we didn't expect this to happen. Is there a problem in the way we used django-crispy-forms in Speedy Net?

[carltongibson]

...what do you want me to do?

I'm trying to understand why the issue arrises. From there we can decide if it's a bug or expected behaviour, and fix it accordingly (either in your project code or here).

Why the issue doesn't occur when the user is logged in is strange. So what's going on there? (If we can explain that then we'll have our answer I think.)

[uri-rodberg]

I'm trying to understand why the issue arrises. From there we can decide if it's a bug or expected behaviour, and fix it accordingly (either in your project code or here).

Why the issue doesn't occur when the user is logged in is strange. So what's going on there? (If we can explain that then we'll have our answer I think.)

@carltongibson Thanks for the explanation. I don't know the reason, but you can see from the tests with 1.7.2 [https://travis-ci.org/speedy-net/speedy-net/builds/477460938], that test_visitor_can_see_feedback_form failed and test_user_can_see_feedback_form didn't fail. With 1.6.1 [https://travis-ci.org/speedy-net/speedy-net/builds/477460948] all the tests didn't fail. These tests are on https://github.com/speedy-net/speedy-net/blob/uri_merge_with_master_2019-01-08_a_copy_with_django-crispy-forms_1.7.2/speedy/core/feedback/tests/test_views.py. I don't know why the visitor tests fail while the (logged in) user tests don't fail but the form is different, the visitor form contains 2 more input fields (sender_name and sender_email) which the logged in user form doesn't contain. There are 6 different tests which fail (in 4 websites) but the forms are similar. I also have a newer version of these tests (in branch staging) - https://github.com/speedy-net/speedy-net/blob/staging/speedy/core/feedback/tests/test_views.py.

[uri-rodberg]

Maybe the difference is in the following code:

            self.helper.add_layout(Row(
                Div('sender_name', css_class='col-md-6'),
                Div('sender_email', css_class='col-md-6'),
            ))

https://github.com/speedy-net/speedy-net/blob/staging/speedy/core/feedback/forms.py

Maybe we must set render_unmentioned_fields to True? And why?

https://stackoverflow.com/questions/11010662/django-crispy-forms-layout-div

[uri-rodberg]

@carltongibson Any comments? Is it a bug in django-crispy-forms or is it the expected behavior? Did we do something wrong in Speedy Net in the way we used django-crispy-forms?

[carltongibson]

Sorry @uri-rodberg, still not at all sure why it's working with an authenticated user and not without?

We don't really have time to dig through the Speedy Net code to see what's going on there.

If you can reduce it to a minimal example it would be possible to comment. Ideally you'd present a minimal unit test showing behaviour that you expect to work but is not.

[uri-rodberg]

@carltongibson class FeedbackForm linked above at https://github.com/speedy-net/speedy-net/blob/staging/speedy/core/feedback/forms.py is a minimal example. I think what causes the code to fail is the code I wrote above:

            self.helper.add_layout(Row(
                Div('sender_name', css_class='col-md-6'),
                Div('sender_email', css_class='col-md-6'),
            ))

This is the main difference between the feedback form for an authenticated user and the feedback form for a non-authenticated user.

Notice we have fields defined in class Meta. Maybe this is what is causing the code to fail. As far as I understand the code it should not be the expected behavior. It looks like a bug. Or maybe some of the constant default values on django-crispy-forms is not intuitive.

[uri-rodberg]

OK, I confirmed with a commit now that if you comment these lines the tests pass and the form contains all the fields we want, but the HTML design is not what we want (because these lines sets the HTML design we want). You can see https://travis-ci.org/speedy-net/speedy-net/builds/485932345 and https://github.com/speedy-net/speedy-net/blob/uri_merge_with_master_2019-01-08_b_copy_with_django-crispy-forms_1.7.2/speedy/core/feedback/forms.py.

[carltongibson]

A minimal example is one using just Django + Crispy Forms. Preferably it would be in the form of a PR including a test case to the project. Failing that a minimal project based on django-admin startproject just adding the forms needed.

In particular the ModelFormWithDefaults isn't something we can take responsibility for.

I'm happy to look at a minimal example but we simply don't have the capacity to look through your project code to work out what's going on.

[uri-rodberg]

@carltongibson It takes time and I don't have much time now to setup a minimal example as you defined. In the meantime I changed the code in Speedy Net so that render_unmentioned_fields will be True every time we use the class FormHelper of django-crispy-forms. This solves the problem for now.

[carltongibson]

OK, glad you found a workaround. I'm going to close pending a reproduce. If you come up with one, happy to reopen then.

[naro]

I would like to reopen this issue. It's very easy to reproduce for me.

Let's have a ModelForm and add a few custom fields to it

class MyModel(Model):
    field1 = models.CharField()

class MyForm(ModelForm):
    extra_field = forms.CharField()
   
    class Meta:
        model = MyModel
        fields = ('field1', 'extra_field')

If you render this form in Crispy Forms < 1.7.0 it works and renders both fields
If you render this form in Crispy Forms >= 1.7.0 it renders field1 only

The reason is commit 6b93e8a which broke it.
The 'extra_field' is neither required or hidden so it depends on render_unmentioned_fields which defaults to False.
Since the field is mentioned in Meta.fields, it should be rendered. See also docs/form_helper.rst

By default django-crispy-forms renders the layout specified if it exists strictly, which means it only renders what the layout mentions unless your form has Meta.fields and Meta.exclude defined, in that case, it uses them. If you want to render unmentioned fields (all form fields), for example, if you are worried about forgetting mentioning them you have to set this property to True. It defaults to False.

I wonder why @ghinch committed this at all. If fields are mentioned in Meta.fields they should be unconditionally rendered, shouldn't they?

[naro]

Actually, it's even worse than I thought. Setting render_unmentioned_fields to True causes all fields rendered through the block above the mentioned code where the fields are defined as a set(form.fields.keys()) which is unordered so the form looks differently (fields are randomly ordered).

[uri-rodberg]

@carltongibson Do you want to comment on what @naro wrote?

[carltongibson]

Ok yep. Let’s reopen to have a look. Thanks @naro.

[uri-rodberg]

Actually, it's even worse than I thought. Setting render_unmentioned_fields to True causes all fields rendered through the block above the mentioned code where the fields are defined as a set(form.fields.keys()) which is unordered so the form looks differently (fields are randomly ordered).

Did PR #952 fix this issue? Maybe it's time to close this issue? @naro @carltongibson

[carltongibson]

Hi @uri-rodberg. Good follow-up. If it's fixed for you in current master, please do close.

We're rolling 1.9 soon™