Skip to content

Conversation

claudep
Copy link
Member

@claudep claudep commented May 28, 2018

Copy link
Member

@charettes charettes May 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it break anything if this is moved to the top? I would assume this module can safely depend on the latter?

Copy link
Member

@charettes charettes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the approach @claudep and it's always great to fix such a long standing bug.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be great to assert the permission and content types were appropriately created as well!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was bitten by this today, and thought it is enough time lost by fellow developers!

Copy link
Member

@charettes charettes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we should eventually warn/error (ImproperlyConfigured) when auth is installed before contenttypes instead to prevent unnecessary double work instead but this could be tackled and discussed somewhere else.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On thing to keep in mind is that this is going to be introducing double work.

@claudep
Copy link
Member Author

claudep commented May 28, 2018

Sure, this reminds us about the wider discussion of INSTALLED_APPS ordering. I agree on discussing this elsewhere, because this bug has waited for too long now.

@timgraham timgraham merged commit bec651a into django:master Jun 4, 2018
@claudep claudep deleted the 10827 branch June 4, 2018 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants