Wicket
  1. Wicket
  2. WICKET-511

New ServletWebRequest.getParameterMap() implementation changed between 1.2.5 and 1.2.6

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Won't Fix
    • Affects Version/s: 1.2.6
    • Fix Version/s: 1.2.7
    • Component/s: wicket
    • Labels:
      None

      Description

      The implementation of ServletWebRequest.getParameterMap() changed between 1.2.5 and 1.2.6. Now it simply calls:

      return new HashMap(httpServletRequest.getParameterMap());

      According to Servlet specification, ServletRequest.getParameterMap() returns map of <String, String[]>. Please note that the value is array of Strings. The new implementation of ServletWebRequest.getParameterMap() is now broken, because in 1.2.5 it returned "Map<String, String>".

        Activity

        Martijn Dashorst made changes -
        Status Open [ 1 ] Closed [ 6 ]
        Resolution Won't Fix [ 2 ]
        Hide
        Martijn Dashorst added a comment -

        We're sorry that this API break came through, but we will not revert it. Currently 1.2.6 is the most widely used Wicket version at this moment, reverting will cause pain to all 1.2.6 upgraders. Furthermore, 1.3 and onward will have this API anyways.

        Upgraders from 1.2.x releases should re-test/recompile their applications to ensure they will not run into this break at runtime.

        Show
        Martijn Dashorst added a comment - We're sorry that this API break came through, but we will not revert it. Currently 1.2.6 is the most widely used Wicket version at this moment, reverting will cause pain to all 1.2.6 upgraders. Furthermore, 1.3 and onward will have this API anyways. Upgraders from 1.2.x releases should re-test/recompile their applications to ensure they will not run into this break at runtime.
        Martijn Dashorst made changes -
        Assignee Martijn Dashorst [ dashorst ]
        Hide
        Martijn Dashorst added a comment -

        So, we are not going to re-/un-/de-/fix this? The damage has been done, no point in reverting a released api break imo. We should just document this and call it a day.

        Show
        Martijn Dashorst added a comment - So, we are not going to re-/un-/de-/fix this? The damage has been done, no point in reverting a released api break imo. We should just document this and call it a day.
        Alastair Maw made changes -
        Summary New ServletWebRequest.getParameterMap() implementation is broken New ServletWebRequest.getParameterMap() implementation changed between 1.2.5 and 1.2.6
        Fix Version/s 1.2.7 [ 12312468 ]
        Hide
        Gwyn Evans added a comment -

        Well, it's now "doing the right thing", but as a matter of policy, had we realised that we were changing the value type, we'd almost certainly not have applied it to 1.2.6 in the same way.

        It would have still been applied to 1.3, though! It's "intended behaviour" there but not for 1.2.6 /purely/ because in a 1.2.x release, we make efforts not to change the API, which /does/ restrict us as to what changes are applied.

        Show
        Gwyn Evans added a comment - Well, it's now "doing the right thing", but as a matter of policy, had we realised that we were changing the value type, we'd almost certainly not have applied it to 1.2.6 in the same way. It would have still been applied to 1.3, though! It's "intended behaviour" there but not for 1.2.6 /purely/ because in a 1.2.x release, we make efforts not to change the API, which /does/ restrict us as to what changes are applied.
        Hide
        Johan Compagner added a comment -

        so we now follow the spec and now we think it is broken???
        we can rollback it for 1.2 but i guess for 1.3 we keep it what it is now. Because it should return String,String[] ....

        Show
        Johan Compagner added a comment - so we now follow the spec and now we think it is broken??? we can rollback it for 1.2 but i guess for 1.3 we keep it what it is now. Because it should return String,String[] ....
        Jan Bareš made changes -
        Field Original Value New Value
        Priority Critical [ 2 ] Trivial [ 5 ]
        Hide
        Jan Bareš added a comment -

        Aha, I should call PageParameters.getString() instead of get(). So maybe this is intended behaviour. Please clarify and close.

        Show
        Jan Bareš added a comment - Aha, I should call PageParameters.getString() instead of get(). So maybe this is intended behaviour. Please clarify and close.
        Hide
        Gwyn Evans added a comment -

        Looks like it was WICKET-291 that changed it - although I doubt it was appreciated then that 1.2.x was 'broken' and this 'fixed' it...

        Show
        Gwyn Evans added a comment - Looks like it was WICKET-291 that changed it - although I doubt it was appreciated then that 1.2.x was 'broken' and this 'fixed' it...
        Jan Bareš created issue -

          People

          • Assignee:
            Martijn Dashorst
            Reporter:
            Jan Bareš
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development