Struts 2
  1. Struts 2
  2. WW-4100

"error" result defined in global-results is ignored when using convention plugin

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.3.14.3
    • Fix Version/s: 2.3.16
    • Component/s: Plugin - Convention
    • Labels:
      None

      Description

      If you have an "error" result defined in global-results in struts.xml and use the convention plugin, the global error result is ignored.

      If you have this:

          <global-results>
            <result name="error" type="dispatcher">/error/strutsError.jsp</result>
          </global-results>
        </package>
      

      The convention plugin will only dispatch to action-error.jsp or action.jsp when the former is not found, ignoring the global-results.

        Activity

        Hide
        Hudson added a comment -

        FAILURE: Integrated in Struts2-JDK6 #809 (See https://builds.apache.org/job/Struts2-JDK6/809/)
        WW-4100 Solves problem with global "error" result when used with Convention plugin (lukaszlenart: rev 1532732)

        • /struts/struts2/trunk/plugins/convention/src/main/java/org/apache/struts2/convention/DefaultResultMapBuilder.java
        • /struts/struts2/trunk/plugins/convention/src/main/java/org/apache/struts2/convention/PackageBasedActionConfigBuilder.java
        • /struts/struts2/trunk/plugins/convention/src/test/java/org/apache/struts2/convention/DefaultResultMapBuilderTest.java
        • /struts/struts2/trunk/plugins/convention/src/test/java/org/apache/struts2/convention/PackageBasedActionConfigBuilderTest.java
        • /struts/struts2/trunk/plugins/convention/src/test/java/org/apache/struts2/convention/actions/result/GlobalResultAction.java
        • /struts/struts2/trunk/plugins/convention/src/test/java/org/apache/struts2/convention/actions/result/GlobalResultOverrideAction.java
        Show
        Hudson added a comment - FAILURE: Integrated in Struts2-JDK6 #809 (See https://builds.apache.org/job/Struts2-JDK6/809/ ) WW-4100 Solves problem with global "error" result when used with Convention plugin (lukaszlenart: rev 1532732) /struts/struts2/trunk/plugins/convention/src/main/java/org/apache/struts2/convention/DefaultResultMapBuilder.java /struts/struts2/trunk/plugins/convention/src/main/java/org/apache/struts2/convention/PackageBasedActionConfigBuilder.java /struts/struts2/trunk/plugins/convention/src/test/java/org/apache/struts2/convention/DefaultResultMapBuilderTest.java /struts/struts2/trunk/plugins/convention/src/test/java/org/apache/struts2/convention/PackageBasedActionConfigBuilderTest.java /struts/struts2/trunk/plugins/convention/src/test/java/org/apache/struts2/convention/actions/result/GlobalResultAction.java /struts/struts2/trunk/plugins/convention/src/test/java/org/apache/struts2/convention/actions/result/GlobalResultOverrideAction.java
        Hide
        ASF subversion and git services added a comment -

        Commit 1532732 from Lukasz Lenart in branch 'struts2/trunk'
        [ https://svn.apache.org/r1532732 ]

        WW-4100 Solves problem with global "error" result when used with Convention plugin

        Show
        ASF subversion and git services added a comment - Commit 1532732 from Lukasz Lenart in branch 'struts2/trunk' [ https://svn.apache.org/r1532732 ] WW-4100 Solves problem with global "error" result when used with Convention plugin
        Hide
        Lukasz Lenart added a comment -

        Patch applied, thanks a lot!

        Show
        Lukasz Lenart added a comment - Patch applied, thanks a lot!
        Hide
        Lukasz Lenart added a comment - - edited

        Mark Woon I will review it soon, I'm just in middle of 2.3.15 release

        Show
        Lukasz Lenart added a comment - - edited Mark Woon I will review it soon, I'm just in middle of 2.3.15 release
        Hide
        Mark Woon added a comment -

        Is my latest patch acceptable? And if so, any chance it'll be applied soon?

        Show
        Mark Woon added a comment - Is my latest patch acceptable? And if so, any chance it'll be applied soon?
        Hide
        Mark Woon added a comment -

        Here's a patch (without the constant) and test cases to back it up.

        Show
        Mark Woon added a comment - Here's a patch (without the constant) and test cases to back it up.
        Hide
        Lukasz Lenart added a comment -

        You can always implement your own ResultMapBuilder and rollback to old behaviour - that's the Struts way

        Show
        Lukasz Lenart added a comment - You can always implement your own ResultMapBuilder and rollback to old behaviour - that's the Struts way
        Hide
        Lukasz Lenart added a comment -

        100% right

        Show
        Lukasz Lenart added a comment - 100% right
        Hide
        Maurizio Cucchiara added a comment -

        My guess is he meant that since Struts ignore global-result, there is a bug.
        No need to introduce a constant to fix a bug.

        For future reference do you guys prefer patches in JIRA over pull requests on GitHub?

        At the moment we prefer JIRA patches.

        Am I right Lukasz Lenart?

        Show
        Maurizio Cucchiara added a comment - My guess is he meant that since Struts ignore global-result, there is a bug. No need to introduce a constant to fix a bug. For future reference do you guys prefer patches in JIRA over pull requests on GitHub? At the moment we prefer JIRA patches. Am I right Lukasz Lenart ?
        Hide
        Mark Woon added a comment -

        Sorry, I'm not following you.

        My assumption is that Struts has been working this way for a long time and I don't want to introduce something that's not backwards compatible. Hence the new constant.

        When you say "basically it's a bug as other options are overwritten" are you referring to the constant I'm proposing? I don't understand what option this new constant overrides.

        Attaching patch now. For future reference do you guys prefer patches in JIRA over pull requests on GitHub?

        Show
        Mark Woon added a comment - Sorry, I'm not following you. My assumption is that Struts has been working this way for a long time and I don't want to introduce something that's not backwards compatible. Hence the new constant. When you say "basically it's a bug as other options are overwritten" are you referring to the constant I'm proposing? I don't understand what option this new constant overrides. Attaching patch now. For future reference do you guys prefer patches in JIRA over pull requests on GitHub?
        Hide
        Lukasz Lenart added a comment -

        Providing additional constant isn't an option - basically it's a bug as other options are overwritten. And please provide patch here as an attachment.

        Show
        Lukasz Lenart added a comment - Providing additional constant isn't an option - basically it's a bug as other options are overwritten. And please provide patch here as an attachment.
        Hide
        Mark Woon added a comment -

        I have. It finds all my other global-results. The only one that it does not find is the "error" global-result.

        Show
        Mark Woon added a comment - I have. It finds all my other global-results. The only one that it does not find is the "error" global-result.
        Hide
        Lukasz Lenart added a comment -

        You must define your package as a default parent package as by default the Convention plugin will use convention-default

        <constant name="struts.convention.default.parent.package" value="my-package"/>
        
        Show
        Lukasz Lenart added a comment - You must define your package as a default parent package as by default the Convention plugin will use convention-default <constant name= "struts.convention.default.parent.package" value= "my-package" />
        Hide
        Mark Woon added a comment -

        Patch provided via GitHub: https://github.com/apache/struts2/pull/6

        Show
        Mark Woon added a comment - Patch provided via GitHub: https://github.com/apache/struts2/pull/6

          People

          • Assignee:
            Lukasz Lenart
            Reporter:
            Mark Woon
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development