-
Notifications
You must be signed in to change notification settings - Fork 29
Web2py-3.x compatibility #61
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
base: dev
Are you sure you want to change the base?
Conversation
|
Here is a patch for PyDAL-20251207.1: This patch fixes an inconsistency in the IS_EMPTY_OR logic of default validators - which is the one breaking issue that currently prevents us from moving to web2py-3.1.1. See also web2py/pydal#752 |
|
Upstream PR submitted here: web2py/pydal#755 |
awjans
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very clean.
| session.flash = messages.password_changed | ||
| if settings.login_after_password_change: | ||
| user = Storage(table_user._filter_fields(user, id=True)) | ||
| user = Storage(filter_fields(table_user, user, allow_id=True)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Should we rename 'table_user' in this method to 'utable' for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so - table_user is a hint that this uses auth.settings.table_user rather than s3db.auth_user (for which utable would typically be used).
Of course, right now both point to the same database table - but the origin of the reference is different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, that makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, that said, it would seem more consistent to rename utable in login_bare (above) to table_user to indicate the fixed origin of the reference.
There is a subtle difference between the two: auth.settings.table_user is a Table (or None), whereas s3db.auth_user returns a Table (or raises). Within AuthS3, self.settings shall be used to access the tables from self.define_tables, not s3db - to ensure the tables belong to the instance (and cannot be manipulated outside of it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...but even though I agree that there is some inconsistency in variable naming here: this is unrelated to the subject of the PR. So I'd say let's test this for its purpose and fix the naming inconsistency later.
This PR adapts Eden to web2py-3.1.1, while retaining web2py-2.27.1 compatibility - so it can be run with both versions.
However, it does not yet list web2py-3.1.1 as supported version because that version (or actually, the PyDAL version it comes with) still has some issues itself, which would need to be fixed upstream or patched locally before we can claim full support.
Further, this still needs to be tested on Python-3.13 (so far only tested on Python-3.11) - so it is not ready for release just yet - but still a critical step into that direction.
For testing (only partially done, Python-3.13 still untested):
modules/updatechk.py-check_web2py_version)models/0000_update_check.pyfiled6dcbef, and update all submodulesFor backwards-compatibility testing (passed/complete, but re-testing welcome):
49bb23cand update all submodulesmodels/0000_update_check.pyfile againThank you!