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

MYFACES-4606 Follow up: do not send values for unselected radio / checkbox elements #634

Closed
wants to merge 1 commit into from

Conversation

volosied
Copy link
Contributor

@volosied volosied requested a review from werpu October 23, 2023 19:51
@werpu
Copy link
Contributor

werpu commented Oct 24, 2023

Not needed: if (item && item.type &&
(item.type.toLowerCase() == "submit" ||
item.type.toLowerCase() == "button" )) {

The original behavior is somewhat different anyway... i just changed it somehow for the new code to cover more elements (aka every element)
Hence I never have issued a pr for the old code it does not expose the problem!

Either way we theoretically could unify the behavior for all branches, the old code and new code that would be feasible, if wanted, given this is a non standard behavior we are free here to do whatever we want!

@volosied
Copy link
Contributor Author

volosied commented Oct 24, 2023

If I update the code to just

    appendIssuingItem: function (item, targetBuf) {
        // if triggered by a Button send it along
        if (item && item.type) {

            // do not append if unselected
            if ((item.type == "checkbox" || item.type == "radio") 
             && !item.checked) {
                return;
            }
            
            //buttons not always have a name unlike inputs
            targetBuf.append(item.id || item.name, item.value);
        }
    },

then I see the form sent as an array.
Screenshot 2023-10-24 at 3 53 59 PM
Looks like it's caused by the preprocessed data also containing the issuing element?

Screenshot 2023-10-24 at 3 54 33 PM

Otherwise, with the existing checks, only true is sent:

Screenshot 2023-10-24 at 3 50 50 PM

@volosied
Copy link
Contributor Author

@werpu What do you recommend here?

@werpu
Copy link
Contributor

werpu commented Oct 25, 2023

Yes the original code only allows submits and buttons to be appended as issuing item, hence I said the problem does not affect the old code, I guess we have to replicate the 4.x behavior into the old code might make sense to have a unified behavior, I will prepare something the next few days!

@volosied
Copy link
Contributor Author

@werpu Any updates? Thank you

@volosied
Copy link
Contributor Author

volosied commented Nov 2, 2023

I would like to merge this if possible soon in order to get new 2.3 3.0 ... etc releases out.

@werpu
Copy link
Contributor

werpu commented Nov 3, 2023

Hi I will work on the issue today!
Sorry was not aware that it is so urgent.
Expect a merge request with the new behavior later today!

@werpu
Copy link
Contributor

werpu commented Nov 3, 2023

I have issued a working code for 3.0, please see:
#638

my integration tests pass, it is basically a backport from 4.0

Please give it a shot and then we can merge if it is ok, this one can now be seen as for 3.x!

I also will issue a bunch of merge requests for the lower versioned branches so that we keep the code in sync, the build for 2.3-next seems to be broken (I have issued one there already)

@volosied
Copy link
Contributor Author

volosied commented Nov 6, 2023

Closing in favor of #638.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants