Remove init_config from LDAP3LoginManager #66
Conversation
The init_config method prevents us from storing LDAP3LoginManager state on the Flask application context, which is needed to support certain patterns of Flask usage. Issue: nickw444#40
Previously, the mock ServerPool was never used with init_app. From the docs, I would have expected MagicMock to implement __iter__, but it wasn't working.
| @@ -49,7 +88,7 @@ class LDAP3LoginManager(object): | |||
| def __init__(self, app=None): | |||
|
|
|||
| self._save_user = None | |||
| self.config = {} | |||
| self.config = dict(_CONFIG_DEFAULTS) | |||
There was a problem hiding this comment.
This will probably lead to the question "why didn't you use a dict literal for _CONFIG_DEFAULTS. The reason is that, in the next PR, I'll change
self.config.update(app.config)to
for k, v in _CONFIG_DEFAULTS:
app.config.setdefault(k, v)in init_app below.
There was a problem hiding this comment.
LGTM! Some great steps toward solving some common issues in this library. Do you have any ideas around a release plan for these changes? I think it will be worth batching the changes part of the upcoming refactor/bug fixes and cut a major API incompatible release. What do you think?
| self.config.setdefault( | ||
| 'LDAP_GET_GROUP_ATTRIBUTES', ldap3.ALL_ATTRIBUTES) | ||
| self.config.setdefault('LDAP_ADD_SERVER', True) | ||
| self.config.update(app.config) |
There was a problem hiding this comment.
Looking forward to config being moved to app.config in the next PR 😄
| @@ -145,6 +124,13 @@ def init_config(self, config): | |||
| use_ssl=self.config['LDAP_USE_SSL'] | |||
| ) | |||
|
|
|||
| if hasattr(app, 'teardown_appcontext'): | |||
| app.teardown_appcontext(self.teardown) | |||
| else: # pragma: no cover | |||
There was a problem hiding this comment.
I wonder if this is something that we can phase out (less need to support old flask versions if we are doing a API incompatible release)?
There was a problem hiding this comment.
Sure. I created #67 to track that.
|
I think bundling together several breaking changes is a good idea. I've created milestone 1.0 to track everything we want to include in the breaking release. |
This is the first PR moving towards addressing #40. Without the need to support
init_configany more, we can unconditionally store the LDAP3LoginManager state on the Flask application context.