Skip to content

DjangoGuardianObjectPermissionsFilter Broken on django 1.11 with python 2.7 #6577

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

Closed
jsharpe opened this issue Apr 8, 2019 · 7 comments · Fixed by #6613
Closed

DjangoGuardianObjectPermissionsFilter Broken on django 1.11 with python 2.7 #6577

jsharpe opened this issue Apr 8, 2019 · 7 comments · Fixed by #6613

Comments

@jsharpe
Copy link

jsharpe commented Apr 8, 2019

This line unconditionally disables django-guardian from being detected with python 2.7 but it should still work with older releases that don't meet the conditions in the comments:

# Guardian 1.5.0, for Django 2.2 is NOT compatible with Python 2.7.

@xordoquy
Copy link
Collaborator

xordoquy commented Apr 8, 2019

We don't test against older Django-guardian versions so there might be other incompatibilities.
If you are confident enough, I'd suggest you override DjangoObjectPermissionsFilter.__init__() to skip the installation check (or perform your own).

@xordoquy xordoquy closed this as completed Apr 8, 2019
@jsharpe
Copy link
Author

jsharpe commented Apr 8, 2019

Can I suggest that you add a warning into the code in the python2 path as currently the code returns a rather confusing error message that guardian is not installed. You have to dig into the code to find out the information about the incompatibility

@xordoquy
Copy link
Collaborator

xordoquy commented Apr 8, 2019

@carltongibson do you think we'll have releases before merging Python 2.7 removal PR ?

@carltongibson
Copy link
Collaborator

Yeah. good. I had basically thought no. But it might be worth throwing this in quick... Thoughts?

@rpkilby
Copy link
Member

rpkilby commented Apr 8, 2019

Almost everything merged since 3.9.2 has either been a docs fix or a minor code cleanup. We could release 3.9.3 if it's not too much trouble.

Maybe change the check to:

if six.PY2 and guardian.VERSION >= (1, 5):
    return False

From #6054, there were issues importing guardian, but since this is a runtime check, it might be fine?

@carltongibson
Copy link
Collaborator

OK. Let’s reopen to assess. I’ve been putting off dropping Python 2, so we may as well look at taking advantage of that.

Happy to look at a PR adjusting the compat code here. I don’t mind doing the release.

@carltongibson carltongibson reopened this Apr 8, 2019
@xordoquy
Copy link
Collaborator

xordoquy commented Apr 8, 2019

It might be more like:

if six.PY2 and django.version >= (2, 2) and guardian.VERSION >= (1, 5):
    return False

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 a pull request may close this issue.

4 participants