Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Override urlconf_module so that Django system checks don't crash. #6719

Merged
merged 2 commits into from Aug 27, 2019

Conversation

@lddubeau
Copy link
Contributor

commented Aug 20, 2019

Summary

Fixes #6717

General checklist

  • I have updated the CHANGELOG.txt

(I don't think this is necessary as this falls under the "Introduced Django 2.2 support." rubric.)

  • I have created backports if necessary

(Not necessary.)

  • I have updated the documentation and/or amended the upgrade section
    if necessary

(Not necessary.)

@coveralls

This comment has been minimized.

Copy link

commented Aug 20, 2019

Coverage Status

Coverage remained the same at 78.215% when pulling b1a17b1 on lddubeau:fix-6717 into 31d1295 on divio:develop.

@FinalAngel

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

@yakky @czpython could you have a look at this, would be awesome to release it in an RC2 :)

Override urlconf_module so that Django system checks don't crash.
Without this fix, Django system checks in Django 2.2 will try to resolve the
"app_resolver" module, which does not exist. Overriding ``urlconf_module``
prevents the issue.

Fixes #6717

@lddubeau lddubeau force-pushed the lddubeau:fix-6717 branch from 6077e24 to b1a17b1 Aug 21, 2019

@lddubeau

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

I've force-pushed a simplification just now. The original FakeModule rigmarole is unnecessary: urlconf_module can evaluate to a list of patterns instead of an object with a urlpatterns attribute.

@yakky

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

@FinalAngel @lddubeau LGTM at first sight, I'm checking a bit more in depth later today
Anyway this should target release/3.7.x branch, right?

@FinalAngel

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

@yakky it's good to target develop, I'll backport then to 3.7.x :)

@yakky
yakky approved these changes Aug 22, 2019
@yakky

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

@lddubeau @FinalAngel LGTM, I also tested in a project with fairly complex apphooks, and everything looks ok

@lddubeau

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

@yakky I also ran the proposed changed with a complex Django project of mine that uses Django CMS. Some of the Django tests don't test apphooks specifically but they do depend on them working properly. The Selenium-based tests access the site as a regular user would, so they also depends on apphooks working fine. All tests passed without problem.

@FinalAngel

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

awesome, I'll be merging this PR tomorrow and prepare an RC2 release with it. @yakky anything else from your end that should also be included in the RC2?

@yakky

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

@FinalAngel I haven't spotted anything else relevant for RC2 👍

@FinalAngel FinalAngel merged commit efbeaa0 into divio:develop Aug 27, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
FinalAngel added a commit to FinalAngel/django-cms that referenced this pull request Aug 27, 2019
Override urlconf_module so that Django system checks don't crash. (d…
…ivio#6719)

* Add a test case for divio#6717.

* Override urlconf_module so that Django system checks don't crash.

Without this fix, Django system checks in Django 2.2 will try to resolve the
"app_resolver" module, which does not exist. Overriding ``urlconf_module``
prevents the issue.

Fixes divio#6717
FinalAngel added a commit that referenced this pull request Aug 27, 2019
Override urlconf_module so that Django system checks don't crash. (#…
…6719) (#6723)

* Add a test case for #6717.

* Override urlconf_module so that Django system checks don't crash.

Without this fix, Django system checks in Django 2.2 will try to resolve the
"app_resolver" module, which does not exist. Overriding ``urlconf_module``
prevents the issue.

Fixes #6717
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.