[jira] Commented: (VELTOOLS-88) LinkTool could easily be modified to support current-request parameter duplication

["Nathan Bubna (JIRA)" <de...@velocity.apache.org>]

    [ https://issues.apache.org/jira/browse/VELTOOLS-88?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12538881 ] 

Nathan Bubna commented on VELTOOLS-88:
--------------------------------------

Nice feature.  It sorta overlaps the self() method when selfIncludeParameters is changed to true, but as the default for that is false, this is a cleaner way to add them than doing $link.params($request.parameterMap), especially with the ability to ignore certain params.  That said, here's some critiques before we put this in:

- The self() method will need to respect the paramsToIgnore when it is configured to include params.
- I'm not thrilled about using addQueryData(name,value) for each individual parameter.  If there's a lot of params, that will needlessly create and discard a lot of LinkTool clones.  I think it would be better if it copied the request.getParameterMap(), removed any params to be ignored from that and then passed the result to params(Map).
- The javadoc references ignore(String) which is not in the patch. 
- addIgnore() should return a copy, not this.  Which means you'll probably need to add a new copyWithIgnore(String) method and make sure that the paramsToIgnore are always copied to new copies.

Sorry if that seems like a lot, but i'd like to avoid introducing inconsistencies in how LinkTool behaves.  speaking of which, i just noticed a few things in it that need attention. :)

let me know if you have questions about my feedback.  i'd be happy to help, as i think this is a nice new feature.

> LinkTool could easily be modified to support current-request parameter duplication
> ----------------------------------------------------------------------------------
>
>                 Key: VELTOOLS-88
>                 URL: https://issues.apache.org/jira/browse/VELTOOLS-88
>             Project: Velocity Tools
>          Issue Type: New Feature
>            Reporter: Christopher Schultz
>            Priority: Trivial
>         Attachments: VELTOOLS-88.diff
>
>
> When implementing a pager a long time ago, I created a subclass of the StrutsLinkTool that added two methods: ignore(String paramName) and addAllParameters(). Together, they could be used to copy all the parameters from the current request into the link currently being built, and you could ignore a parameter from the current request using the ignore method.
> I hadn't seen any demand for such a feature out in the wild, so I kept it to myself.
> Now that I'm looking into Struts 2, I can see that their link builder tool has the same capability. Since that team decided to add this feature, I'm offering to add it to the LinkTool, which will provide the same capability to all subclasses such as StrutsLinkTool and SecureLinkTool.
> The change would not affect any existing code out in the wild: this would merely be a new method which supports the new feature. Its use is entirely optional and should not disturb any other code.
> The behavior of the Struts 2 link builder is to replace any current-request parameter explicitly set in the link builder, while my implementation requires that you explicitly ignore those parameters. My enhancement can follow either strategy (and I'd appreciate feedback on this item).

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org