lingthio / Flask-User

Customizable User Authorization & User Management: Register, Confirm, Login, Change username/password, Forgot password and more.
http://flask-user.readthedocs.io/
MIT License
1.06k stars 293 forks source link

Remodel initialization #76

Open Xender opened 9 years ago

Xender commented 9 years ago

Could you consider remodelling initialization so it will follow two-phase model, like Flask-Security and a lot of other Flask extensions?

Here's an example of the pattern being used in application (FlaskBB): Phase 1 - Module where all extensions' objects are defined Phase 2 - Initializing all the extensions in function called form app factory function

AFAIK, it's currently impossible to do the two-phase initialization of Flask-User without making a circular import, because of object-creation-time dependencies:

And here's how Flask-Security facilitates that.

Would you accept a pull request if I were to implement the change proposed here?

neurosnap commented 9 years ago

Greetings, just to clarify, what is the issue you are having specifically? Would you like user_manager to be defined in an extension.py file like Flask-WTF, Flask-SQLAlchemy, Flask-Mail, etc., and then initialize them in the main app factory function?

This is how I do it:

user.py

from .ext import db
from .models import User

db_adapter = SQLAlchemyAdapter(db, User)
user_manager = UserManager(db_adapter)

ext.py

from flask_sqlalchemy import SQLAlchemy

db = SQLAlchemy()

app.py

from .ext import db
from .user import user_manager

def create_app():
    app = Flask(__name__)
    configure_extensions(app)
    return app

def configure_extensions(app):
    db.init_app(app)
    user_manager.init_app(app)
Xender commented 9 years ago

Hi, thanks for fast reply.

I've already seen the pattern you described above in Flask-User-starter-app.

You basically nailed it - I wanted to have all the Flask extension objects defined in one place. It's more a matter of code organisation than anything else for me.

Do you see any reason against making this change in the initialization routine that I'm not aware of? I am aware that it may be not needed for anything besides enabling this kind of 2-phase extensions init.

As far as I know, some extensions like db = SQLAlchemy() objects need to be defined in some global place and available early on, because a lot of other files depend on them (all the models).

I don't know if this is the case with UserManager object - does it need to accessed in code after initialization, or is it "fire and forget" kind of extension object?

I am aware that there is another way to access extension objects after the app is initialised, flask.current_app.extensions['user'] (one needs application context to use it, though), so as I said, as far as I understand, it's just a matter of code organisation.

lingthio commented 9 years ago

I believe you are requesting to move the kwargs initialization from __init__() to init_app(). This seems a reasonable request.

neurosnap commented 9 years ago

This seems rather simple and a change that shouldn't break any existing apps.

Here is an example with tests passing in py2.7 and py3.4:

https://github.com/neurosnap/Flask-User/commit/89aeef671d9ef9baa0ce424c129b2c74834899f3

It might be a good idea to add a test or two for this new change, but this is a good start

lingthio commented 9 years ago

Thanks @neurosnap . I've moved ALL the custom parameters to init_app() and added a kwargs param to __init__(). I will release this soon.

Xender commented 9 years ago

Thank you for such fast responses! You two are golden. :)

lingthio commented 9 years ago

I've released v0.6.4. @Xender : I have only tested this with __init__(db_adapter, app, ...), so please test this with init_app(app, db_adapter, ...) and let us know.

Also note the parameter order reversal. Needed this to ensure backwards compatibility.

neurosnap commented 9 years ago

@Xender @lingthio Happy to help, I think this is a good feature to add.

Sebi2020 commented 4 years ago

I've released v0.6.4. @Xender : I have only tested this with __init__(db_adapter, app, ...), so please test this with init_app(app, db_adapter, ...) and let us know.

Also note the parameter order reversal. Needed this to ensure backwards compatibility.

This is neither fixeed nor released (v.1.0.2.2). The UserManager class requires you to hand over app,db and user classes, which does not comply to flask extension rules.

See also: https://flask.palletsprojects.com/en/1.1.x/extensiondev/?highlight=extension%20development#the-extension-code And: https://flask.palletsprojects.com/en/1.1.x/extensiondev/?highlight=extension%20development#approved-extensions

1wonjae commented 3 years ago

This is neither fixeed nor released (v.1.0.2.2). The UserManager class requires you to hand over app,db and user classes, which does not comply to flask extension rules.

I believe you can pass any arguments and the initialization does not take place until init_app() is called. So perhaps it is a simple issue of introducing default values at __init__()?