[
https://issues.apache.org/jira/browse/WICKET-5648?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14068529#comment-14068529
]
Thibault Kruse commented on WICKET-5648:
----------------------------------------
Hi Martin, a couple of things I notice in the diff:
in private void remove(final Cookie cookie), you now save the cookie before
setting the values to be written? That seems like a bug, it would not delete
the values to be deleted
public load and save methods could be documented with "internal", or at least
warn that values with colons will not be split correctly.
+ key = Strings.replaceAll(key, ".", "..").toString();
not sure about toString() here, seems unnecessary
taking the first char of a FormComponent.VALUE_SEPARATOR is okay now, but dodgy
code. Not sure why CookieUtils cannot merely define a SEPARATOR constant of
it's own, that would clean up things. Else, it should at least define a static
char constant with value FormComponent.VALUE_SEPARATOR.charAt(0) , IMO
return Strings.split(value, FormComponent.VALUE_SEPARATOR.charAt(0));
> CookieUtils - add #loadValues(), make #getCookie() public, properly
> initialize from the defaults
> ------------------------------------------------------------------------------------------------
>
> Key: WICKET-5648
> URL: https://issues.apache.org/jira/browse/WICKET-5648
> Project: Wicket
> Issue Type: Improvement
> Reporter: Thibault Kruse
> Assignee: Martin Grigorov
> Priority: Minor
> Fix For: 7.0.0-M3, 6.17.0
>
>
> Hi, not sure even whether CookieUtils is supposed to be used outside wicket.
> But if so, it has some API flaws.
> The CookieUtils class has
> public final void save(String key, final String... values)
> but no *public* load method to load the saved multiple values. Clients can
> load the whole string and split themselves, but that's dirty. And using
> FormComponent.VALUE_SEPARATOR seems wicket-specific anyway (and is not safe
> against values with that separator), so maybe that method should be
> protected, not public.
> The code
> cookie.setSecure(false);
> in save() also seems dodgy, but seems to have no effect (defaultSettings
> still work).
> Finally it is a bit weird that there is no access to the underlying Cookie
> itself, with it's getDomain() etc methods. For Developers it might be nice to
> work with the cookie avoiding the boilerplate code, so maybe getCookie()
> could be made public instead of private
--
This message was sent by Atlassian JIRA
(v6.2#6252)
