Skip to content
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

CRITICAL BUG in FormAlchemy v1.4.3: _sanitize_select_options() in helpers.py #52

Open
coredumperror opened this issue Dec 14, 2013 · 3 comments

Comments

@coredumperror
Copy link
Contributor

I can't even begin to imagine what this thing is supposed to be doing, but what it's actually doing is destroying long <select> lists.

Here's the code for the function:

def _sanitize_select_options(options):
    if isinstance(options, (list, tuple)):
        if _only_contains_leaves(options) and len(options) >= 2:
            return (options[1], options[0])
        else:
            return [_sanitize_select_options(option) for option in options]
    return options

Notice that if the options variable is a list of length >= 2, with elements which are not also lists (e.g. a list of several strings), it converts options into (options[1], options[0]).

Why in the world does it do this? This results in every list of 2+ options being incorrectly rendered into a <select>.

I'm no expert when it comes to recursive functions, but it appears that (besides destroying every option list) this function doesn't do anything. When I made what appeared to be the most sensible fix the remove the bug, I realized that the function stopped having any effect.

I'd suggest either removing _sanitize_select_options(), or re-writing it to do what it was originally intended to do, whatever that was. Until that time, though, FormAlchemy v1.4.3 is completely worthless. I was forced to downgrade to v1.4.2.

@maxiberta
Copy link

Downgrading to 1.4.2 exposes issue #40 (cannot import exceptions from sqlalchemy), but at least there's a trivial fix for that.

Also noticed that several unit tests in formalchemy/tests/test_dates.py pass in 1.4.2 but fail in 1.4.3. In particular, _sanitize_select_options() breaks TimeFieldRenderer, and as a result only 2 options are shown in Hour, Minute and Second fields. Looks like 1.4.3 was released with little QA :-(

@maxiberta
Copy link

Commit a2d26dc (Support for webhelpers optgroups) introduced _sanitize_select_options(). There's a descriptive commit message but I can't figure out what it means.

@smurfix
Copy link
Contributor

smurfix commented May 21, 2014

Apparently the author of this patch (a) never used "raw" options, but always (key,value) tuples, (b) didn't think, (c) didn't run the testsuite (it appears to have been broken before this commit …)

I fixed the formalchemy code to get the testsuite working again, and to always use tuples internally; see #60.
I probably also should add a reversal of this patch (and a few more testcases for this) to my patchset, but time is limited. :-/

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

No branches or pull requests

3 participants