You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@struts.apache.org by pb...@apache.org on 2007/07/02 06:03:18 UTC
svn commit: r552396 -
/struts/struts1/trunk/core/src/main/java/org/apache/struts/chain/commands/AbstractPopulateActionForm.java
Author: pbenedict
Date: Sun Jul 1 21:03:17 2007
New Revision: 552396
URL: http://svn.apache.org/viewvc?view=rev&rev=552396
Log:
STR-1674: Check cancellation first and possibly skip population
Modified:
struts/struts1/trunk/core/src/main/java/org/apache/struts/chain/commands/AbstractPopulateActionForm.java
Modified: struts/struts1/trunk/core/src/main/java/org/apache/struts/chain/commands/AbstractPopulateActionForm.java
URL: http://svn.apache.org/viewvc/struts/struts1/trunk/core/src/main/java/org/apache/struts/chain/commands/AbstractPopulateActionForm.java?view=diff&rev=552396&r1=552395&r2=552396
==============================================================================
--- struts/struts1/trunk/core/src/main/java/org/apache/struts/chain/commands/AbstractPopulateActionForm.java (original)
+++ struts/struts1/trunk/core/src/main/java/org/apache/struts/chain/commands/AbstractPopulateActionForm.java Sun Jul 1 21:03:17 2007
@@ -45,21 +45,26 @@
*/
public boolean execute(ActionContext actionCtx)
throws Exception {
- // Is there a form bean for this request?
+
+ ActionConfig actionConfig = actionCtx.getActionConfig();
ActionForm actionForm = actionCtx.getActionForm();
+ // First determine if the request was cancelled
+ handleCancel(actionCtx, actionConfig, actionForm);
+
+ // Is there a form bean for this request?
if (actionForm == null) {
return (false);
}
- // Reset the form bean property values
- ActionConfig actionConfig = actionCtx.getActionConfig();
+ // If request is cancelled, form manipulation is prevented
+ if (actionCtx.getCancelled().booleanValue()) {
+ return (false);
+ }
+ // Reset and repopulate the form bean property values
reset(actionCtx, actionConfig, actionForm);
-
populate(actionCtx, actionConfig, actionForm);
-
- handleCancel(actionCtx, actionConfig, actionForm);
return (false);
}
Re: svn commit: r552396 - /struts/struts1/trunk/core/src/main/java/org/apache/struts/chain/commands/AbstractPopulateActionForm.java
Posted by Paul Benedict <pb...@apache.org>.
I tested your suggestion. It works great. I am 100% for this.
Niall Pemberton wrote:
> On 7/6/07, Paul Benedict <pb...@apache.org> wrote:
>> Niall Pemberton wrote:
>> > IMO people who want to move forward should use Struts2 - Struts 1
>> > shouldn't break compatibilty lightly - I see no great benefit to
>> > changing how this has worked for 6 years and just pain for anyone who
>> > has happened to rely on it. Also with the Command Chain introduced in
>> > Struts 1.3 its possible to easily provide both new functionality and
>> > compatibility with different Command implementations. So I'm still
>> > against this change as it is now.
>> >
>> > Niall
>> >
>> I am not interested in lightning bolt changes for 1.x. People will move
>> to 2.0 as necessary, but I imagine 1.0 will live on for a very long time
>> and upgrades will still be wanted without a revolution. Granted to your
>> point, the change is not fixing anything considered a blocker/emergency,
>> but I do believe this patches a hole in the way Struts could be misused.
>> The real problem behind collecting data during cancellation is the false
>> assurance that the form was completed correctly. However, this argument
>> is only as strong as the perceived danger of accidentally shooting
>> oneself in the foot :-) Because manipulating form data in a cancellation
>> is probably highly irregular, I think its justified to block off this
>> route without affecting well-structured apps.
>>
>> While I would like to see the change stand as is, you've made me think
>> of another alternative: Limit the change to when the form is present,
>> *validation is true,* but canceled. Because my entire emphasis is on
>> preventing the manipulation on a validated form, I find this alternative
>> to be worthier. Your thoughts appreciated.
>
> IMO "sometimes populate on cancel" is probably the worst possible
> option - incompatible and incosistent!
>
> I still am against breaking compatibility over this - but I will stop
> objecting if theres a way for users to configure for the old behaviour
> - AFAIK we can set properties in the chain config - so one possible
> option would be to add a "populateOnCancel" option to
> "AbstractPopulateActionForm" and in the chain-config.xml something
> like:
>
> <command
>
> className="org.apache.struts.chain.commands.servlet.PopulateActionForm"
> populateOnCancel="false" />
>
> Niall
>
>> Paul
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: svn commit: r552396 - /struts/struts1/trunk/core/src/main/java/org/apache/struts/chain/commands/AbstractPopulateActionForm.java
Posted by Niall Pemberton <ni...@gmail.com>.
On 7/6/07, Paul Benedict <pb...@apache.org> wrote:
> Niall Pemberton wrote:
> > IMO people who want to move forward should use Struts2 - Struts 1
> > shouldn't break compatibilty lightly - I see no great benefit to
> > changing how this has worked for 6 years and just pain for anyone who
> > has happened to rely on it. Also with the Command Chain introduced in
> > Struts 1.3 its possible to easily provide both new functionality and
> > compatibility with different Command implementations. So I'm still
> > against this change as it is now.
> >
> > Niall
> >
> I am not interested in lightning bolt changes for 1.x. People will move
> to 2.0 as necessary, but I imagine 1.0 will live on for a very long time
> and upgrades will still be wanted without a revolution. Granted to your
> point, the change is not fixing anything considered a blocker/emergency,
> but I do believe this patches a hole in the way Struts could be misused.
> The real problem behind collecting data during cancellation is the false
> assurance that the form was completed correctly. However, this argument
> is only as strong as the perceived danger of accidentally shooting
> oneself in the foot :-) Because manipulating form data in a cancellation
> is probably highly irregular, I think its justified to block off this
> route without affecting well-structured apps.
>
> While I would like to see the change stand as is, you've made me think
> of another alternative: Limit the change to when the form is present,
> *validation is true,* but canceled. Because my entire emphasis is on
> preventing the manipulation on a validated form, I find this alternative
> to be worthier. Your thoughts appreciated.
IMO "sometimes populate on cancel" is probably the worst possible
option - incompatible and incosistent!
I still am against breaking compatibility over this - but I will stop
objecting if theres a way for users to configure for the old behaviour
- AFAIK we can set properties in the chain config - so one possible
option would be to add a "populateOnCancel" option to
"AbstractPopulateActionForm" and in the chain-config.xml something
like:
<command
className="org.apache.struts.chain.commands.servlet.PopulateActionForm"
populateOnCancel="false" />
Niall
> Paul
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: svn commit: r552396 - /struts/struts1/trunk/core/src/main/java/org/apache/struts/chain/commands/AbstractPopulateActionForm.java
Posted by Paul Benedict <pb...@apache.org>.
Niall Pemberton wrote:
> IMO people who want to move forward should use Struts2 - Struts 1
> shouldn't break compatibilty lightly - I see no great benefit to
> changing how this has worked for 6 years and just pain for anyone who
> has happened to rely on it. Also with the Command Chain introduced in
> Struts 1.3 its possible to easily provide both new functionality and
> compatibility with different Command implementations. So I'm still
> against this change as it is now.
>
> Niall
>
I am not interested in lightning bolt changes for 1.x. People will move
to 2.0 as necessary, but I imagine 1.0 will live on for a very long time
and upgrades will still be wanted without a revolution. Granted to your
point, the change is not fixing anything considered a blocker/emergency,
but I do believe this patches a hole in the way Struts could be misused.
The real problem behind collecting data during cancellation is the false
assurance that the form was completed correctly. However, this argument
is only as strong as the perceived danger of accidentally shooting
oneself in the foot :-) Because manipulating form data in a cancellation
is probably highly irregular, I think its justified to block off this
route without affecting well-structured apps.
While I would like to see the change stand as is, you've made me think
of another alternative: Limit the change to when the form is present,
*validation is true,* but canceled. Because my entire emphasis is on
preventing the manipulation on a validated form, I find this alternative
to be worthier. Your thoughts appreciated.
Paul
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: svn commit: r552396 - /struts/struts1/trunk/core/src/main/java/org/apache/struts/chain/commands/AbstractPopulateActionForm.java
Posted by Niall Pemberton <ni...@gmail.com>.
On 7/6/07, Paul Benedict <pb...@apache.org> wrote:
> The ticket issue shows the conversation starting without a form and then
> growing to include the form. So I believe it was appropriate to address
> both situations, which I think are okay.
I had missed that in the comments :(
> First, in all the years of
> development of Struts, I have never come across one user mail or program
> code where people are asking to retrieve values on cancellation. Please
> find one for me so I can be corrected. In fact, I can't think of one
> framework which allows this anyway. I put this in the "bad practice"
> department and believe this should not be supported in anyway -- and the
> current situation is an oversight. The action is canceled, the form is
> part of the action, and so the form has no job which is very much
> implied by the canceling of validation.
>
> Now granted, there seems to be concern here with you and Martin.
> However, I think this is a justified change, and going from one version
> to the next does not have to be 100% backwards compatible .. just about
> 98 or 99%, in my opinion. Remember, this codebase has no 2.0 version --
> that's done by the WW port -- so if things are going to move forward,
> and perhaps dream to fix some rather odd use cases, I find this
> appropriate just as much as Struts 2.1 alters some functionality of
> Struts 2.0.
IMO people who want to move forward should use Struts2 - Struts 1
shouldn't break compatibilty lightly - I see no great benefit to
changing how this has worked for 6 years and just pain for anyone who
has happened to rely on it. Also with the Command Chain introduced in
Struts 1.3 its possible to easily provide both new functionality and
compatibility with different Command implementations. So I'm still
against this change as it is now.
Niall
> Paul
>
> Niall Pemberton wrote:
> > I don't have a problem with setting the "cancellation" attribute where
> > there is no form - but this changes goes further than whats mentioned
> > in STR-1674 with the form no longer being populated when "cancelled".
> > This breaks compatibility with previous Struts versions and I'm
> > against it.
> >
> > Niall
> >
> > On 7/2/07, pbenedict@apache.org <pb...@apache.org> wrote:
> >> Author: pbenedict
> >> Date: Sun Jul 1 21:03:17 2007
> >> New Revision: 552396
> >>
> >> URL: http://svn.apache.org/viewvc?view=rev&rev=552396
> >> Log:
> >> STR-1674: Check cancellation first and possibly skip population
> >>
> >> Modified:
> >>
> >> struts/struts1/trunk/core/src/main/java/org/apache/struts/chain/commands/AbstractPopulateActionForm.java
> >>
> >>
> >> Modified:
> >> struts/struts1/trunk/core/src/main/java/org/apache/struts/chain/commands/AbstractPopulateActionForm.java
> >>
> >> URL:
> >> http://svn.apache.org/viewvc/struts/struts1/trunk/core/src/main/java/org/apache/struts/chain/commands/AbstractPopulateActionForm.java?view=diff&rev=552396&r1=552395&r2=552396
> >>
> >> ==============================================================================
> >>
> >> ---
> >> struts/struts1/trunk/core/src/main/java/org/apache/struts/chain/commands/AbstractPopulateActionForm.java
> >> (original)
> >> +++
> >> struts/struts1/trunk/core/src/main/java/org/apache/struts/chain/commands/AbstractPopulateActionForm.java
> >> Sun Jul 1 21:03:17 2007
> >> @@ -45,21 +45,26 @@
> >> */
> >> public boolean execute(ActionContext actionCtx)
> >> throws Exception {
> >> - // Is there a form bean for this request?
> >> +
> >> + ActionConfig actionConfig = actionCtx.getActionConfig();
> >> ActionForm actionForm = actionCtx.getActionForm();
> >>
> >> + // First determine if the request was cancelled
> >> + handleCancel(actionCtx, actionConfig, actionForm);
> >> +
> >> + // Is there a form bean for this request?
> >> if (actionForm == null) {
> >> return (false);
> >> }
> >>
> >> - // Reset the form bean property values
> >> - ActionConfig actionConfig = actionCtx.getActionConfig();
> >> + // If request is cancelled, form manipulation is prevented
> >> + if (actionCtx.getCancelled().booleanValue()) {
> >> + return (false);
> >> + }
> >>
> >> + // Reset and repopulate the form bean property values
> >> reset(actionCtx, actionConfig, actionForm);
> >> -
> >> populate(actionCtx, actionConfig, actionForm);
> >> -
> >> - handleCancel(actionCtx, actionConfig, actionForm);
> >>
> >> return (false);
> >> }
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: svn commit: r552396 - /struts/struts1/trunk/core/src/main/java/org/apache/struts/chain/commands/AbstractPopulateActionForm.java
Posted by Paul Benedict <pb...@apache.org>.
The ticket issue shows the conversation starting without a form and then
growing to include the form. So I believe it was appropriate to address
both situations, which I think are okay. First, in all the years of
development of Struts, I have never come across one user mail or program
code where people are asking to retrieve values on cancellation. Please
find one for me so I can be corrected. In fact, I can't think of one
framework which allows this anyway. I put this in the "bad practice"
department and believe this should not be supported in anyway -- and the
current situation is an oversight. The action is canceled, the form is
part of the action, and so the form has no job which is very much
implied by the canceling of validation.
Now granted, there seems to be concern here with you and Martin.
However, I think this is a justified change, and going from one version
to the next does not have to be 100% backwards compatible .. just about
98 or 99%, in my opinion. Remember, this codebase has no 2.0 version --
that's done by the WW port -- so if things are going to move forward,
and perhaps dream to fix some rather odd use cases, I find this
appropriate just as much as Struts 2.1 alters some functionality of
Struts 2.0.
Paul
Niall Pemberton wrote:
> I don't have a problem with setting the "cancellation" attribute where
> there is no form - but this changes goes further than whats mentioned
> in STR-1674 with the form no longer being populated when "cancelled".
> This breaks compatibility with previous Struts versions and I'm
> against it.
>
> Niall
>
> On 7/2/07, pbenedict@apache.org <pb...@apache.org> wrote:
>> Author: pbenedict
>> Date: Sun Jul 1 21:03:17 2007
>> New Revision: 552396
>>
>> URL: http://svn.apache.org/viewvc?view=rev&rev=552396
>> Log:
>> STR-1674: Check cancellation first and possibly skip population
>>
>> Modified:
>>
>> struts/struts1/trunk/core/src/main/java/org/apache/struts/chain/commands/AbstractPopulateActionForm.java
>>
>>
>> Modified:
>> struts/struts1/trunk/core/src/main/java/org/apache/struts/chain/commands/AbstractPopulateActionForm.java
>>
>> URL:
>> http://svn.apache.org/viewvc/struts/struts1/trunk/core/src/main/java/org/apache/struts/chain/commands/AbstractPopulateActionForm.java?view=diff&rev=552396&r1=552395&r2=552396
>>
>> ==============================================================================
>>
>> ---
>> struts/struts1/trunk/core/src/main/java/org/apache/struts/chain/commands/AbstractPopulateActionForm.java
>> (original)
>> +++
>> struts/struts1/trunk/core/src/main/java/org/apache/struts/chain/commands/AbstractPopulateActionForm.java
>> Sun Jul 1 21:03:17 2007
>> @@ -45,21 +45,26 @@
>> */
>> public boolean execute(ActionContext actionCtx)
>> throws Exception {
>> - // Is there a form bean for this request?
>> +
>> + ActionConfig actionConfig = actionCtx.getActionConfig();
>> ActionForm actionForm = actionCtx.getActionForm();
>>
>> + // First determine if the request was cancelled
>> + handleCancel(actionCtx, actionConfig, actionForm);
>> +
>> + // Is there a form bean for this request?
>> if (actionForm == null) {
>> return (false);
>> }
>>
>> - // Reset the form bean property values
>> - ActionConfig actionConfig = actionCtx.getActionConfig();
>> + // If request is cancelled, form manipulation is prevented
>> + if (actionCtx.getCancelled().booleanValue()) {
>> + return (false);
>> + }
>>
>> + // Reset and repopulate the form bean property values
>> reset(actionCtx, actionConfig, actionForm);
>> -
>> populate(actionCtx, actionConfig, actionForm);
>> -
>> - handleCancel(actionCtx, actionConfig, actionForm);
>>
>> return (false);
>> }
>>
>>
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: svn commit: r552396 - /struts/struts1/trunk/core/src/main/java/org/apache/struts/chain/commands/AbstractPopulateActionForm.java
Posted by Niall Pemberton <ni...@gmail.com>.
I don't have a problem with setting the "cancellation" attribute where
there is no form - but this changes goes further than whats mentioned
in STR-1674 with the form no longer being populated when "cancelled".
This breaks compatibility with previous Struts versions and I'm
against it.
Niall
On 7/2/07, pbenedict@apache.org <pb...@apache.org> wrote:
> Author: pbenedict
> Date: Sun Jul 1 21:03:17 2007
> New Revision: 552396
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=552396
> Log:
> STR-1674: Check cancellation first and possibly skip population
>
> Modified:
> struts/struts1/trunk/core/src/main/java/org/apache/struts/chain/commands/AbstractPopulateActionForm.java
>
> Modified: struts/struts1/trunk/core/src/main/java/org/apache/struts/chain/commands/AbstractPopulateActionForm.java
> URL: http://svn.apache.org/viewvc/struts/struts1/trunk/core/src/main/java/org/apache/struts/chain/commands/AbstractPopulateActionForm.java?view=diff&rev=552396&r1=552395&r2=552396
> ==============================================================================
> --- struts/struts1/trunk/core/src/main/java/org/apache/struts/chain/commands/AbstractPopulateActionForm.java (original)
> +++ struts/struts1/trunk/core/src/main/java/org/apache/struts/chain/commands/AbstractPopulateActionForm.java Sun Jul 1 21:03:17 2007
> @@ -45,21 +45,26 @@
> */
> public boolean execute(ActionContext actionCtx)
> throws Exception {
> - // Is there a form bean for this request?
> +
> + ActionConfig actionConfig = actionCtx.getActionConfig();
> ActionForm actionForm = actionCtx.getActionForm();
>
> + // First determine if the request was cancelled
> + handleCancel(actionCtx, actionConfig, actionForm);
> +
> + // Is there a form bean for this request?
> if (actionForm == null) {
> return (false);
> }
>
> - // Reset the form bean property values
> - ActionConfig actionConfig = actionCtx.getActionConfig();
> + // If request is cancelled, form manipulation is prevented
> + if (actionCtx.getCancelled().booleanValue()) {
> + return (false);
> + }
>
> + // Reset and repopulate the form bean property values
> reset(actionCtx, actionConfig, actionForm);
> -
> populate(actionCtx, actionConfig, actionForm);
> -
> - handleCancel(actionCtx, actionConfig, actionForm);
>
> return (false);
> }
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org