Follow up on https://issues.apache.org/jira/browse/MYFACES-4606

[werpu]

Fix for MyFaces 4606, checkboxes and radio buttons are not appended automatically if unchecked!

[werpu]

Backport from 4.0

Give it a shot, we can merge it, from my side it works!
(tested it with my it test suite)

[werpu]

does it work for you?

[volosied]

@werpu I'm getting the following error in my test case:

Error Message: (intermediate value).toLowerCase is not a function

--------------------------------------------------------
Calling class:myfaces._impl.xhrCore._AjaxRequestQuirks
Calling function:send
Error Name: sendError

--------------------------------------------------------
Note, this message is only sent, because project stage is development and no other error listeners are registered. 

xhtml:

      <h:form id="form1">
         <h:selectBooleanCheckbox id="checkbox" value="#{testBean.checked}">
            <f:ajax render="output"/>
         </h:selectBooleanCheckbox>
         <h:outputText id="output" value = "#{testBean.checked}" />
      </h:form>
      ```

[werpu]

mhh let me check quickly! I think I found it, sorry my mistake!

[werpu]

Fixed it, sorry for the issue, I guess I have to recheck my integration tests apparently this code was not triggered by them!
(the 4.x codebase has a dedicated unit test so it is covered there definitely)

[volosied]

Yep, it's fixed now. Thanks!

However, I'm still seeing the an array sent for the issuing element?

Looks like the data is attached via both appendIssuingItem & the _preprocessedData (mentioned here: #634 (comment))

[volosied]

I'm seeing it's caused by

ajaxUtils.encodeSubmittableFields(ret, form, null);
https://github.com/apache/myfaces/blob/3.0.x/api/src/main/javascript/META-INF/resources/myfaces/_impl/core/Impl.js#L111C19-L111C42

https://github.com/apache/myfaces/blob/3.0.x/api/src/main/javascript/META-INF/resources/myfaces/_impl/xhrCore/_AjaxUtils.js#L38

Can we add some check to avoid encoding the the data twice?

[werpu]

I have repushed some code, please test it again (I did add the check blindly because it is getting late here).
A test on the appendIssuingItem should suffice, since the formData object already has the encoded fields in!
Give it a shot!

if(targetBuf.hasKey(identifier)) { //already processed within the values
        return;
    }

is basically the fix for this, no need to append the issuing item if the item already is encoded.
The targetBuf is of Type FormData which has the method hasKey!

ret = this._Lang.createFormDataDecorator(new Array());
_AJAXUTIL.encodeSubmittableFields(ret, this._sourceForm, this._partialIdsArray);
if (this._source && myfacesOptions && myfacesOptions.form)
_AJAXUTIL.appendIssuingItem(this._source, ret);

[volosied]

Thanks for looking. I hoped it would work, too, but it doesn't, unfortunately. Problem looks to be here:

getFormData:function () {
    var formDataDecorator = this._Lang.createFormDataDecorator(jsf.getViewState(this._sourceForm)); <-- 
    this._AJAXUTIL.appendIssuingItem(this._source, formDataDecorator);
    return formDataDecorator;
},

When jsf.getViewState(this._sourceForm) is called, it encodes the form into a string. formDataDecorator's idx variable is then empty, so the new check fails.

Perhaps we would wait to encode things later? Or append the issuing item first and then add the viewstate related data, if it wasn't already added?

We can just continue with this more this week. Thanks again for your help here!

[werpu]

Hi, I fixed the check on FormData level, apparently the hasKey function only checked for newly appended values not existing data passed into the existing formData object, this should fix the issue for good, a preliminary test locally now works for me, and all integration tests still pass. I will check whether this issue also is present for the 4.0 code, and if all is well on both code levels we can cherry pick the fixes for the older branches!

The 4.0 code is not affected it has a proper check code in place which actually works (checks the underlying FormData object), so all is well there. I will prepare the backports tomorrow, if the 3.0 code now works for you guys!

[volosied]

Hi, the changes look good to me. Both scenarios I've tested pass.

Let me run the full TCK and (hopefully) we merge this in!

[werpu]

Any news on the TCK @volosied ?

[volosied]

Hi sorry for the delay -- I used the current 3.0.x branch along with your commits to test the TCK. I was hoping to create a 3.0 release soon ( along with the other branches).

Turns out there's a few failures, but I haven't had a moment to investigate them. It's possible they aren't related to your changes, but I'll get back to you soon.

[melloware]

i thought the 3.0.x branch we decided was dead?

[volosied]

We've still been patching fixes, and it's a version our product still supports. And since the code is mostly the same as 2.3, I expect the TCK failures to also exist in that branch.

It doesn't take too much time to create a release.

[werpu]

I can make a quick pull request for 2.3 (next has one already), if wanted, I just wanted to wait for the results for 3.0 before moving forward with the lower branches! And yes the code between 2.x and 3.0 is mostly the same, so it should be a simple cherry pick!

[werpu]

Hi i added a small fix from an issue which was reported from the Tobago guys!
The fix for 4.0+ will come soon, it just needs final Tobago testing, but their testcase is fixed by this fix.
Since the issue was in the new code of 4606 i did not put it into a separate pr!

[werpu]

@volosied there were indeed smaller issues, which the Tobago guys reported my, all of them are now backported to the 3.x branch, please give it another shot!

[volosied]

Thanks - I'll test it out again. Give me a few days!

[werpu]

Thanks for testing and closing!