Struts 1
  1. Struts 1
  2. STR-2208

Multiple classes using deprecated DefinitionsUtil class

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 1.1 RC2
    • Fix Version/s: None
    • Component/s: Tiles 1 Plugin
    • Labels:
      None
    • Environment:
      Operating System: Windows XP
      Platform: PC

      Description

      TilesRequestProcessor, DefinitionDispatcherAction and TilesPlugin are all
      referencing static methods in DefinitionsUtil which are deprecated.

        Issue Links

          Activity

          Hide
          sean schofield added a comment -

          Created an attachment (id=12838)
          Patch for DefinitionDispatcherAction

          Show
          sean schofield added a comment - Created an attachment (id=12838) Patch for DefinitionDispatcherAction
          Hide
          sean schofield added a comment -

          Created an attachment (id=12839)
          Patch for TilesRequestProcessor

          Show
          sean schofield added a comment - Created an attachment (id=12839) Patch for TilesRequestProcessor
          Hide
          sean schofield added a comment -

          The two patch files provided will fix most of the problem. I will see if I
          can also provide a patch for TilesPlugin.

          Show
          sean schofield added a comment - The two patch files provided will fix most of the problem. I will see if I can also provide a patch for TilesPlugin.
          Hide
          sean schofield added a comment -

          DefinitionDispatcherAction was using the now deprecated DefinitionsUtil method
          to store an action definition in the request for later use. The
          TilesRequestProcessor was using DefinitionsUtil to retrieve that same
          definition.

          Since TilesRequestProcessor was already checking to see if the path of the
          ActionForward corresponded to a definition, there was really no need to have
          DefinitionDispatcherAction store the actual reference to the tiles definition
          in the request.

          The solution was to just have DefinitionDispatcherAction return an
          ActionForward with the path of the tile definition. TilesRequestProcessor
          continues as before but now does not bother to ask the deprecated
          DefinitionsUtil class if there is a definition stored in the request.

          Show
          sean schofield added a comment - DefinitionDispatcherAction was using the now deprecated DefinitionsUtil method to store an action definition in the request for later use. The TilesRequestProcessor was using DefinitionsUtil to retrieve that same definition. Since TilesRequestProcessor was already checking to see if the path of the ActionForward corresponded to a definition, there was really no need to have DefinitionDispatcherAction store the actual reference to the tiles definition in the request. The solution was to just have DefinitionDispatcherAction return an ActionForward with the path of the tile definition. TilesRequestProcessor continues as before but now does not bother to ask the deprecated DefinitionsUtil class if there is a definition stored in the request.
          Hide
          sean schofield added a comment -

          Created an attachment (id=12888)
          Fixes deprecated refs in TilesPlugin

          Show
          sean schofield added a comment - Created an attachment (id=12888) Fixes deprecated refs in TilesPlugin
          Hide
          sean schofield added a comment -

          Third and final patch has been submitted. Once patches are applied, bug
          should be considered complete.

          Show
          sean schofield added a comment - Third and final patch has been submitted. Once patches are applied, bug should be considered complete.
          Hide
          Niall Pemberton added a comment -

          The proposed change to TilesRequestProcessor would cause backward compatibility
          issues if anyone is relying on the fact that storing a defintion in the Request
          under the ACTION_DEFINITION key causes it to be used. Personally I wasn't aware
          of this behaviour - but since its there should we continue to support it or
          not?

          If the answer is that we should support it then we probably need to move the
          get/set/removeActionDefinition methods to TilesUtils - and in that scenario
          then the simplest change to DefinitionDispatcherAction would be to just switch
          from using DefinitionsUtil to TilesUtil.

          If we don't care about continuing to support it, then Sean's changes look good.
          It would be good if someone who knows more about the thinking behind that bit
          of the code could comment.

          I also wasn't aware that Tiles still supported using init-params in the web.xml
          until I saw Sean's patch for TilesPlugin. Since most of the configuration that
          used to use init-params has been moved into the struts-config.xml (including
          Tiles through the PlugIn) then I think rather than change the code as Sean's
          proposing, we should just remove it.

          Niall

          Show
          Niall Pemberton added a comment - The proposed change to TilesRequestProcessor would cause backward compatibility issues if anyone is relying on the fact that storing a defintion in the Request under the ACTION_DEFINITION key causes it to be used. Personally I wasn't aware of this behaviour - but since its there should we continue to support it or not? If the answer is that we should support it then we probably need to move the get/set/removeActionDefinition methods to TilesUtils - and in that scenario then the simplest change to DefinitionDispatcherAction would be to just switch from using DefinitionsUtil to TilesUtil. If we don't care about continuing to support it, then Sean's changes look good. It would be good if someone who knows more about the thinking behind that bit of the code could comment. I also wasn't aware that Tiles still supported using init-params in the web.xml until I saw Sean's patch for TilesPlugin. Since most of the configuration that used to use init-params has been moved into the struts-config.xml (including Tiles through the PlugIn) then I think rather than change the code as Sean's proposing, we should just remove it. Niall
          Hide
          sean schofield added a comment -

          I propose we patch DefinitionDispatcherAction for starters. It doesn't seem
          to have any issues associated with its fix.

          I don't know enough about TilesPlugin to comment. I can't say why it was
          doing what it was. My patch just removes the deprecation problem. If folks
          want to scrap that piece alltogether that is fine with me.

          I'm not too worried about the backwards compatability issue with the fix for
          TilesRequestProcessor. The ACTION_DEFINITION key being used is defined in
          DefinitionsUtil. Since that entire class is deprecated, the key is going to
          be going away soon anyways. I vote to apply this patch as well.

          Show
          sean schofield added a comment - I propose we patch DefinitionDispatcherAction for starters. It doesn't seem to have any issues associated with its fix. I don't know enough about TilesPlugin to comment. I can't say why it was doing what it was. My patch just removes the deprecation problem. If folks want to scrap that piece alltogether that is fine with me. I'm not too worried about the backwards compatability issue with the fix for TilesRequestProcessor. The ACTION_DEFINITION key being used is defined in DefinitionsUtil. Since that entire class is deprecated, the key is going to be going away soon anyways. I vote to apply this patch as well.
          Hide
          Ted Husted added a comment -

          Where do we stand on the Tiles Support in Struts-Chain, which will be the
          RequestProcessor for 1.3.x?

          http://svn.apache.org/viewcvs.cgi/struts/trunk/contrib/struts-chain/

          Is "going away soon" 1.3.x?

          -Ted.

          Show
          Ted Husted added a comment - Where do we stand on the Tiles Support in Struts-Chain, which will be the RequestProcessor for 1.3.x? http://svn.apache.org/viewcvs.cgi/struts/trunk/contrib/struts-chain/ Is "going away soon" 1.3.x? -Ted.
          Hide
          sean schofield added a comment -

          Ted,

          I was referring to the backwards compatability issues raised by Niall. He
          raises the concern that someone may be storing an action definition in the
          request using the ACTION_DEFINITION key. If so, the patch related to the
          RequestProcessor will cause this mechanism to fail.

          It seems to me that not many people are doing this, and the key's primary
          purpose is no longer necessary (even though technically it could be considered
          a backwards compatability problem.)

          My point is that the key itself is deprecated and part of a deprecated class.
          This is what I mean by "its going away soon" (not the request processor.) So
          if someone is using this key in a current application, they are getting
          deprecation warnings. In my mind, it would not be unfair to remove this key
          (and all references to the deprecated DefinitionsUtil class) in the next
          release.

          You can't remove DefinitionsUtil until you make these fixes I'm suggesting.
          Once these fixes are made, then I believe we could nix the DefinitionsUtil
          class. Dropping DefintionsUtil can be done now or later depending on the
          groups policies towards deprecation and how long is appropriate.

          If we're not going to make the patches now, I recommend creating a new bug
          entry for dropping DefinitionsUtil and making it dependent on this bug entry.
          Then both can be done at the same time.

          Show
          sean schofield added a comment - Ted, I was referring to the backwards compatability issues raised by Niall. He raises the concern that someone may be storing an action definition in the request using the ACTION_DEFINITION key. If so, the patch related to the RequestProcessor will cause this mechanism to fail. It seems to me that not many people are doing this, and the key's primary purpose is no longer necessary (even though technically it could be considered a backwards compatability problem.) My point is that the key itself is deprecated and part of a deprecated class. This is what I mean by "its going away soon" (not the request processor.) So if someone is using this key in a current application, they are getting deprecation warnings. In my mind, it would not be unfair to remove this key (and all references to the deprecated DefinitionsUtil class) in the next release. You can't remove DefinitionsUtil until you make these fixes I'm suggesting. Once these fixes are made, then I believe we could nix the DefinitionsUtil class. Dropping DefintionsUtil can be done now or later depending on the groups policies towards deprecation and how long is appropriate. If we're not going to make the patches now, I recommend creating a new bug entry for dropping DefinitionsUtil and making it dependent on this bug entry. Then both can be done at the same time.

            People

            • Assignee:
              Unassigned
              Reporter:
              sean schofield
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:

                Development