Skip to content
This repository has been archived by the owner on Jul 30, 2024. It is now read-only.

changes required to work with recent version of flask-login #393

Closed
wants to merge 7 commits into from

Conversation

asmodehn
Copy link

@asmodehn asmodehn commented Jun 6, 2015

This likely is BREAKING with the packaged version of flask-login.

We probably want a way to get backward compatibility with the released version of flask-login... or do/should we expect flask-login to implement that backward compatibility ?

@coveralls
Copy link

Coverage Status

Coverage decreased (-28.09%) to 66.56% when pulling c9edf54 on asmodehn:develop into 33252ae on mattupstate:develop.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-28.09%) to 66.56% when pulling c9edf54 on asmodehn:develop into 33252ae on mattupstate:develop.

@jonafato
Copy link
Contributor

I've been thinking about how this should be handled for a while. Instead of forcing users to use specific versions of Flask-Security and Flask-Login together, what if the use of this method / property included a check for callable? Something along the lines of

callable(current_user.is_authenticated) and current_user.is_authenticated() or current_user.is_authenticated

may allow for backward and forward compatibility, which will be especially useful while developers are in the process of upgrading their libraries' versions.

@asmodehn
Copy link
Author

callable made problems in the test templates ( accessing callable() from jinja).
Instead I opted for a quick - hopefully temporary - patch ( forward compat with recent flask-login ) of the AnonymourUserMixin just after importing it in core.py

@asmodehn
Copy link
Author

Apparently this still needs some py3 fixes... but I don't know much about it.
If anybody is interested I can give permissions on my fork.
Just let me know in a comment here.

@jonafato
Copy link
Contributor

These failing Python 3 tests appear to be caused by an unrelated dependency update. Rebasing should take care of them.

merging bcrypt dependency fix
@fuhrysteve
Copy link
Contributor

This is a fun solution! I hate to shoot it down, but I strongly believe we should not monkey patch Flask-Login to inject graceful backwards compatibility into it; neither should we make any attempt to support multiple versions of Flask-Login in this library.

If we were going to go with this solution, is_authenticated_compat would belong in the Flask-Login project and not this one. I think you're going to get a lot of push back on that from @maxcountryman (and for good reason).

I recommend we solve backward compatibility issues with dependencies

The master branch needs this in requirements.txt ASAP as a hotfix / minor release:

Flask-Login>=0.1.3,<0.3

The develop branch needs this in requirements.txt, and the develop branch needs to treat is_authenticated as a boolean instead of a callable:

Flask-Login>=0.3

The next time there is a major release, flask-security simply drops support for earlier versions of Flask-Login through plain and simple dependency management.

Let's not over engineer this! This is precisely what dependency management exists for: managing backward compatibility. There is simply no reason that future versions of Flask-Security need to support past versions of Flask-Login

@maxcountryman
Copy link

@fuhrysteve 👍 well said!

@fuhrysteve
Copy link
Contributor

I recommend these two solutions:

Immediate bug fix release (master branch): #423

Proper fix for upstream (develop branch): #417

@GregoryVigoTorres
Copy link

Perfectly reasonable.
Do you think this should be mentioned in the docs or CHANGES?

@asmodehn
Copy link
Author

@fuhrysteve: damn, of course, why didnt I think about that...

@fuhrysteve
Copy link
Contributor

@GregoryVigoTorres Probably something in the changelog.. but I think we need to wait for @mattupstate to get back from his honeymoon (oct 5 according to twitter) as he's the maintainer and the only one who can decide how to do releases

@GregoryVigoTorres
Copy link

Of course, some things are more important.
It's a little unfortunate all this happened right when the poor guy was getting married.

@fuhrysteve
Copy link
Contributor

At least the workaround is pretty simple: pip install "flask-login<0.3"

@fake-name
Copy link

Is there any way you folks can get the content of requirements.txt updated to reflect this issue? Right now, if you pip install flask-security, you wind up with a broken install (because it installs flask-login 0.3.0).

That'd be nice, and I wouldn't have to independently discover this issue that way.

@jonafato
Copy link
Contributor

jonafato commented Nov 5, 2015

Thank you for the backward-compatible fix. #417 has been merged, fixing this issue and dropping support for older releases of Flask-Login, so I'm closing this PR.

@jonafato jonafato closed this Nov 5, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants