OFBiz
  1. OFBiz
  2. OFBIZ-278

Relative paths for form definitions passed to the HtmlFormWrapper constructor limit reusability of scripts.

    Details

      Description

      Copy of http://jira.undersunconsulting.com/browse/OFBIZ-603 from Chris Howe.

      ==================================================

      component://order/webapp/ordermgr/WEB-INF/actions/order/OrderDeliveryScheduleInfo.bsh
      line 53:

      uses "/order/OrderDeliveryScheduleForms.xml" as a parameter.

      Because of this every app that calls this beanshell script must have the forms.xml file in it's path. This is extremely trivial at the moment, but will cause some people some trouble once the order application is updated to use mainDecoratorLocation variable

      1. htmlformwrapper.patch
        2 kB
        Jacopo Cappellato
      2. htmlformwrapper.patch
        1 kB
        Chris Howe

        Activity

        Hide
        Jacopo Cappellato added a comment -

        This is a more general issue.
        We should fix all the bsh scripts that use the HtmlFormWrapper constructor to put in the context an instance of a form wrapper.
        I did a search for the "new HtmlFormWrapper(" sting and I've found a lot of scripts.

        Show
        Jacopo Cappellato added a comment - This is a more general issue. We should fix all the bsh scripts that use the HtmlFormWrapper constructor to put in the context an instance of a form wrapper. I did a search for the "new HtmlFormWrapper(" sting and I've found a lot of scripts.
        Hide
        Jacques Le Roux added a comment -

        So the HtmlFormWrapper is deprecated ?

        Show
        Jacques Le Roux added a comment - So the HtmlFormWrapper is deprecated ?
        Hide
        Jacopo Cappellato added a comment -

        Jacques,

        I wouldn't say that the HtmlFormWrapper is deprecated, there are probably situation where putting the forms in the context by declaring them in the screen definition wouldn't work well... however I think that whenever possible, we should avoid to create the form wrapper in the bsh script.

        Show
        Jacopo Cappellato added a comment - Jacques, I wouldn't say that the HtmlFormWrapper is deprecated, there are probably situation where putting the forms in the context by declaring them in the screen definition wouldn't work well... however I think that whenever possible, we should avoid to create the form wrapper in the bsh script.
        Hide
        Jacques Le Roux added a comment -

        Thanks Jacopo,

        I asked this question because for the moment HtmlFormWrapper is only used in BSH with harcoded pathes. Which, if I have well understood, is not a best practice at all.

        Show
        Jacques Le Roux added a comment - Thanks Jacopo, I asked this question because for the moment HtmlFormWrapper is only used in BSH with harcoded pathes. Which, if I have well understood, is not a best practice at all.
        Hide
        David E. Jones added a comment -

        I don't see what the issue is. Even in screen definitions the form location and name are usually "hard coded".

        This is the old way of doing it, used when the form widget existed and the screen widget did not. So yes, these could certainly be updated to use the screen widget instead of including forms in an FTL file, but it's not a huge deal.

        Show
        David E. Jones added a comment - I don't see what the issue is. Even in screen definitions the form location and name are usually "hard coded". This is the old way of doing it, used when the form widget existed and the screen widget did not. So yes, these could certainly be updated to use the screen widget instead of including forms in an FTL file, but it's not a huge deal.
        Hide
        Chris Howe added a comment -

        It's not that it's hard coded, it's that it is a relative path. it is using using /order/... instead of component://order/webapp/order/... Therefore if another component wants to reuse that script, it has to have a component://my_component/webapp/my_webapp/order/OrderDeliveryScheduleForms.xml

        Show
        Chris Howe added a comment - It's not that it's hard coded, it's that it is a relative path. it is using using /order/... instead of component://order/webapp/order/... Therefore if another component wants to reuse that script, it has to have a component://my_component/webapp/my_webapp/order/OrderDeliveryScheduleForms.xml
        Hide
        David E. Jones added a comment -

        If you want to make stuff reusable, definitely refactor it to include from a screen definition rather than leaving it this way. Unless for some reason that isn't possible?

        Show
        David E. Jones added a comment - If you want to make stuff reusable, definitely refactor it to include from a screen definition rather than leaving it this way. Unless for some reason that isn't possible?
        Hide
        Chris Howe added a comment -

        HtmlFormWrapper cannot handle the resource path component://.... as input.

        So, the first thing that needs to be solved is that HtmlFormWrapper needs to be able to do that.

        After that, it's not an issue of refactoring to parameterizing the resource into the screen widget (while that might be beneficial in some cases), but rather refactoring :

        /order/OrderDeliveryScheduleForms.xml
        to:
        component://order/webapp/ordermgr/order/OrderDeliveryScheduleForms.xml

        This issue is simply broadened because the format HtmlFormWrapper() uses a relative path at least 53 times throughout the applications folder.

        Show
        Chris Howe added a comment - HtmlFormWrapper cannot handle the resource path component://.... as input. So, the first thing that needs to be solved is that HtmlFormWrapper needs to be able to do that. After that, it's not an issue of refactoring to parameterizing the resource into the screen widget (while that might be beneficial in some cases), but rather refactoring : /order/OrderDeliveryScheduleForms.xml to: component://order/webapp/ordermgr/order/OrderDeliveryScheduleForms.xml This issue is simply broadened because the format HtmlFormWrapper() uses a relative path at least 53 times throughout the applications folder.
        Hide
        Jacques Le Roux added a comment -

        Chris,

        May we keep this issue open ?

        Show
        Jacques Le Roux added a comment - Chris, May we keep this issue open ?
        Hide
        Chris Howe added a comment -

        I haven't had a chance to create/test out the proposed solution yet. Applications outside of the component still fail when trying reuse the script, so this should stay open until there's a solution. This is no longer a "trivial" issue since the order component does use the parameters.mainDecorator approach.

        Show
        Chris Howe added a comment - I haven't had a chance to create/test out the proposed solution yet. Applications outside of the component still fail when trying reuse the script, so this should stay open until there's a solution. This is no longer a "trivial" issue since the order component does use the parameters.mainDecorator approach.
        Hide
        Chris Howe added a comment -

        htmlformwrapper.patch

        This appears to solve the problem by using the component:// syntax instead of a relative path.

        All instances of the relative path in bsh files would need to be modified as well and I will be happy to do that after feedback.

        Show
        Chris Howe added a comment - htmlformwrapper.patch This appears to solve the problem by using the component:// syntax instead of a relative path. All instances of the relative path in bsh files would need to be modified as well and I will be happy to do that after feedback.
        Hide
        Jacopo Cappellato added a comment -

        Chris,

        still not tested but your patch seems really interesting.
        However it would be really great to make it backward compatible (this would also make the second step of fixing all the relative paths less urgent); is it something you could add to your patch?

        Thanks

        Show
        Jacopo Cappellato added a comment - Chris, still not tested but your patch seems really interesting. However it would be really great to make it backward compatible (this would also make the second step of fixing all the relative paths less urgent); is it something you could add to your patch? Thanks
        Hide
        Jacopo Cappellato added a comment -

        Chris,

        don't worry about the enhancement for backward compatibility, I will attach here a modified patch within a few minutes.

        Show
        Jacopo Cappellato added a comment - Chris, don't worry about the enhancement for backward compatibility, I will attach here a modified patch within a few minutes.
        Hide
        Jacopo Cappellato added a comment -

        This is the backward compatible version of Chris's patch: with this the HtmlFormWrapper object can be created using the old relative paths or the new component:// sintax.
        Please review because I'd like to commit it soon.

        Thanks

        Show
        Jacopo Cappellato added a comment - This is the backward compatible version of Chris's patch: with this the HtmlFormWrapper object can be created using the old relative paths or the new component:// sintax. Please review because I'd like to commit it soon. Thanks
        Hide
        Chris Howe added a comment -

        Looks good.

        Show
        Chris Howe added a comment - Looks good.
        Hide
        Jacques Le Roux added a comment -

        Yes better indeed

        Show
        Jacques Le Roux added a comment - Yes better indeed
        Hide
        Jacopo Cappellato added a comment -

        Rev. 505882

        Show
        Jacopo Cappellato added a comment - Rev. 505882

          People

          • Assignee:
            Jacopo Cappellato
            Reporter:
            Marco Risaliti
          • Votes:
            1 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development