Backport OFBIZ-11317

[Jacques Le Roux]

Le 15/02/2020 à 17:39, Michael Brohl a écrit :
> UrlRegexpTransform should also be moved to the framework then, it does not belong in the applications/product component when used elsewhere.

To be checked, I mean why is it there and not in framework? It could miss in product...
I also wonder why we have all these separated freemarkerTransforms.properties. Why not put most, if not all, in framework? I may miss something
though, I did not dig deep there.

Jacques

>
> Michael Brohl
>
> ecomify GmbH - www.ecomify.de
>
> Am 15.02.20 um 17:20 schrieb Michael Brohl:
>> Hi Jacques,
>>
>> thanks for confirming my concerns.
>>
>> This is why I am advocating for longer review periods and RTC over CTR for new features/improvements, especially in the core functionality with
>> potential to break something. More eyes help to detect potential problems and flaws, they just need some time.
>>
>> I will try to provide a patch for a proper deprecated OfbizUrlTransform class (delegating everything to UrlRegexpTransform and with annotations)
>> which we will need for a proper backport where functional changes should not be implemented. User implementations might rely on the
>> OfbizUrlTransform implementation.
>>
>> I've assigned https://issues.apache.org/jira/browse/OFBIZ-11229 to me for it.
>>
>> Thanks,
>>
>> Michael Brohl
>>
>> ecomify GmbH - www.ecomify.de
>>
>>
>> Am 15.02.20 um 13:27 schrieb Jacques Le Roux:
>>> Hi Michael,
>>>
>>> It's a long answer, but I think it's worth it.
>>>
>>> 1St, technically it's not my commit but James's ;)
>>>
>>> But I asked you, and reviewed/agreed with James's commit, so here we go.
>>>
>>> Your intuition was right, there is a problem with this patch. In some place, like at least themes/tomahawk/template/AppBarClose.ftl the CSRF token
>>> is not generated. Because in those places OfbizUrlTransform is used.
>>>
>>> An immediate solution is to add the CSRF token generation in OfbizUrlTransform class. I just did so at OFBIZ-11317
>>>
>>> But I agree that it's not satisfactory and your proposition to deprecate OfbizUrlTransform in favour of UrlRegexpTransform makes sense to me.
>>> That's mostly why I asked you a second time.
>>>
>>> Let's go a bit in the past. With  OFBIZ-5312, which was a major change started in 2013, we notably introduced UrlRegexpTransform.
>>> We used a Svn feature branch for that: http://svn.apache.org/viewvc?view=revision&revision=r1535432
>>>
>>> It's unrelated, but while at it since it was quite unfortunate, a conflict between Anil and Paul Piper (I was then working with Paul) started and
>>> I had to find a solution which ended with ecomseo.
>>> Fortunately OFBiz is flexible enough and eventually ecomseo is now a simple clone of the ecommerce webapp (like exampleext is for the example
>>> webapp). Everyone can pick her/his preferred one :).
>>>
>>> Now about OfbizUrlTransform vs UrlRegexpTransform: after OFBIZ-5312 UrlRegexpTransform was the only one used for ofbizUrl macro.
>>> That stopped for good reason (Framework dependency on product component) in 2018 with Deepak's work on OFBIZ-10463.
>>> It also shows that during this period (3 years) nobody encountered and issue with UrlRegexpTransform used to generate ofbizUrl macro.
>>>
>>> So yes, we should definitely deprecate OfbizUrlTransform in favour of UrlRegexpTransform. But before that we need to be sure that
>>> UrlRegexpTransform would not miss a feature of OfbizUrlTransform.
>>> With OFBIZ-11229, Nicolas already started to merge UrlRegexpTransform and OfbizUrlTransform classes. This issue is still open and we should use it
>>> to reach our goal.
>>> Maybe it's already OK and we have just to check. For that I'll soon attach a diff of the 2 classes at OFBIZ-11229
>>>
>>> Thanks to care!
>>>
>>> I wish a nice weekend to all of you.
>>>
>>> Jacques
>>>
>>> Le 14/02/2020 à 14:47, Michael Brohl a écrit :
>>>> Please have a closer look at your commit, the controlPath functionality is implemented in the UrlRegexpTransform class...
>>>>
>>>> The implementing class for ofbizUrl is configured in the freemarkerTransforms.properties which is OfbizurlTransform for framework/webapp but
>>>> UrlRegexpTransform for applications/product.
>>>>
>>>> This seems inconsistent to me.
>>>>
>>>> Thanks,
>>>>
>>>> Michael Brohl
>>>>
>>>> ecomify GmbH - www.ecomify.de
>>>>
>>>>
>>>> Am 14.02.20 um 14:24 schrieb Jacques Le Roux:
>>>>> Le 13/02/2020 à 13:06, Michael Brohl a écrit :
>>>>>> There's currently only few questions left for me: we have one configuration which uses org.apache.ofbiz.webapp.ftl.OfbizUrlTransform for
>>>>>> ofbizUrl and this would not support the controlPath configuration if I don't miss something. Wouldn't it be better to change this to
>>>>>> UrlRegexpTransform and deprecate OfbizUrlTransform?
>>>>>>
>>>>>> UrlRegexpTransform is located inside the product component but the macro is used in several other components as well. I think it belongs to the
>>>>>> framework/webapp instead of applications/product. What do you think?
>>>>>
>>>>> Hi Michael,
>>>>>
>>>>> I'm not sure to understand. Are you speaking about OFBiz OOTB?
>>>>>
>>>>> Because the changes in OFBIZ-11317 are only related to ofbizUrl, hence org.apache.ofbiz.webapp.ftl.OfbizUrlTransform, so of course ofbizUrl
>>>>> supports the controlPath configuration. Also why would you want to replace OfbizUrlTransform by UrlRegexpTransform?
>>>>>
>>>>> Jacques
>>>>>
>>>>
>>
>

[Jacques Le Roux]

Michael,

Inline...

Le 15/02/2020 à 17:20, Michael Brohl a écrit :
> Hi Jacques,
>
> thanks for confirming my concerns.
>
> This is why I am advocating for longer review periods and RTC over CTR for new features/improvements, especially in the core functionality with
> potential to break something. More eyes help to detect potential problems and flaws, they just need some time.

I think I need to explain why I'm for CTR over RTC. I have always a sour taste about abandoned patches which become unusable. I don't put the fault on
anybody. We have all our priorities. But I bet that if ever we change for RTC this aspect will worse. Also I'm sure that RTC would never prevent
errors. It would just slow the commit process. The earlier things are in the trunk, the earlier we spot issues. And we have around 3 years for that,
before a package is released. I think we can agree to disagree. And if it's not enough for you, you may ask the community about that.


>
> I will try to provide a patch for a proper deprecated OfbizUrlTransform class (delegating everything to UrlRegexpTransform and with annotations)
> which we will need for a proper backport where functional changes should not be implemented. User implementations might rely on the
> OfbizUrlTransform implementation.
>
> I've assigned https://issues.apache.org/jira/browse/OFBIZ-11229 to me for it.

Great, looking forward

Jacques

>
> Thanks,
>
> Michael Brohl
>
> ecomify GmbH - www.ecomify.de
>
>
> Am 15.02.20 um 13:27 schrieb Jacques Le Roux:
>> Hi Michael,
>>
>> It's a long answer, but I think it's worth it.
>>
>> 1St, technically it's not my commit but James's ;)
>>
>> But I asked you, and reviewed/agreed with James's commit, so here we go.
>>
>> Your intuition was right, there is a problem with this patch. In some place, like at least themes/tomahawk/template/AppBarClose.ftl the CSRF token
>> is not generated. Because in those places OfbizUrlTransform is used.
>>
>> An immediate solution is to add the CSRF token generation in OfbizUrlTransform class. I just did so at OFBIZ-11317
>>
>> But I agree that it's not satisfactory and your proposition to deprecate OfbizUrlTransform in favour of UrlRegexpTransform makes sense to me.
>> That's mostly why I asked you a second time.
>>
>> Let's go a bit in the past. With  OFBIZ-5312, which was a major change started in 2013, we notably introduced UrlRegexpTransform.
>> We used a Svn feature branch for that: http://svn.apache.org/viewvc?view=revision&revision=r1535432
>>
>> It's unrelated, but while at it since it was quite unfortunate, a conflict between Anil and Paul Piper (I was then working with Paul) started and I
>> had to find a solution which ended with ecomseo.
>> Fortunately OFBiz is flexible enough and eventually ecomseo is now a simple clone of the ecommerce webapp (like exampleext is for the example
>> webapp). Everyone can pick her/his preferred one :).
>>
>> Now about OfbizUrlTransform vs UrlRegexpTransform: after OFBIZ-5312 UrlRegexpTransform was the only one used for ofbizUrl macro.
>> That stopped for good reason (Framework dependency on product component) in 2018 with Deepak's work on OFBIZ-10463.
>> It also shows that during this period (3 years) nobody encountered and issue with UrlRegexpTransform used to generate ofbizUrl macro.
>>
>> So yes, we should definitely deprecate OfbizUrlTransform in favour of UrlRegexpTransform. But before that we need to be sure that
>> UrlRegexpTransform would not miss a feature of OfbizUrlTransform.
>> With OFBIZ-11229, Nicolas already started to merge UrlRegexpTransform and OfbizUrlTransform classes. This issue  is still open and we should use it
>> to reach our goal.
>> Maybe it's already OK and we have just to check. For that I'll soon attach a diff of the 2 classes at OFBIZ-11229
>>
>> Thanks to care!
>>
>> I wish a nice weekend to all of you.
>>
>> Jacques
>>
>> Le 14/02/2020 à 14:47, Michael Brohl a écrit :
>>> Please have a closer look at your commit, the controlPath functionality is implemented in the UrlRegexpTransform class...
>>>
>>> The implementing class for ofbizUrl is configured in the freemarkerTransforms.properties which is OfbizurlTransform for framework/webapp but
>>> UrlRegexpTransform for applications/product.
>>>
>>> This seems inconsistent to me.
>>>
>>> Thanks,
>>>
>>> Michael Brohl
>>>
>>> ecomify GmbH - www.ecomify.de
>>>
>>>
>>> Am 14.02.20 um 14:24 schrieb Jacques Le Roux:
>>>> Le 13/02/2020 à 13:06, Michael Brohl a écrit :
>>>>> There's currently only few questions left for me: we have one configuration which uses org.apache.ofbiz.webapp.ftl.OfbizUrlTransform for
>>>>> ofbizUrl and this would not support the controlPath configuration if I don't miss something. Wouldn't it be better to change this to
>>>>> UrlRegexpTransform and deprecate OfbizUrlTransform?
>>>>>
>>>>> UrlRegexpTransform is located inside the product component but the macro is used in several other components as well. I think it belongs to the
>>>>> framework/webapp instead of applications/product. What do you think?
>>>>
>>>> Hi Michael,
>>>>
>>>> I'm not sure to understand. Are you speaking about OFBiz OOTB?
>>>>
>>>> Because the changes in OFBIZ-11317 are only related to ofbizUrl, hence org.apache.ofbiz.webapp.ftl.OfbizUrlTransform, so of course ofbizUrl
>>>> supports the controlPath configuration. Also why would you want to replace OfbizUrlTransform by UrlRegexpTransform?
>>>>
>>>> Jacques
>>>>
>>>
>

[Jacques Le Roux]

Hi All,

I forgot the essential:  to remove "/control/" in all URLs, similarly as it's done in ecomseo webapp.

The best advocate for that is Paul (Foxworthy):

    <<The word "control" in the middle of all our APIs is a technical implementation detail, and it's just noise for all our users all of the time.>>[1]

It's the most important point for UrlRegexpTransform usage: we shall remove "/control/" in all URLs . I have created OFBIZ-11354 for that.

Also, somehow related, long ago Paul suggested to add the "Canonical link element" notion  in OFBIZ-5312: https://s.apache.org/93dl5

Jacques
1] https://markmail.org/message/gzsdbqn3dyfpfetc

Le 15/02/2020 à 19:10, Jacques Le Roux a écrit :

> Michael,
>
> Inline...
>
> Le 15/02/2020 à 17:20, Michael Brohl a écrit :
>> Hi Jacques,
>>
>> thanks for confirming my concerns.
>>
>> This is why I am advocating for longer review periods and RTC over CTR for new features/improvements, especially in the core functionality with
>> potential to break something. More eyes help to detect potential problems and flaws, they just need some time.
>
> I think I need to explain why I'm for CTR over RTC. I have always a sour taste about abandoned patches which become unusable. I don't put the fault
> on anybody. We have all our priorities. But I bet that if ever we change for RTC this aspect will worse. Also I'm sure that RTC would never prevent
> errors. It would just slow the commit process. The earlier things are in the trunk, the earlier we spot issues. And we have around 3 years for that,
> before a package is released. I think we can agree to disagree. And if it's not enough for you, you may ask the community about that.
>
>
>>
>> I will try to provide a patch for a proper deprecated OfbizUrlTransform class (delegating everything to UrlRegexpTransform and with annotations)
>> which we will need for a proper backport where functional changes should not be implemented. User implementations might rely on the
>> OfbizUrlTransform implementation.
>>
>> I've assigned https://issues.apache.org/jira/browse/OFBIZ-11229 to me for it.
>
> Great, looking forward
>
> Jacques
>
>
>>
>> Thanks,
>>
>> Michael Brohl
>>
>> ecomify GmbH - www.ecomify.de
>>
>>
>> Am 15.02.20 um 13:27 schrieb Jacques Le Roux:
>>> Hi Michael,
>>>
>>> It's a long answer, but I think it's worth it.
>>>
>>> 1St, technically it's not my commit but James's ;)
>>>
>>> But I asked you, and reviewed/agreed with James's commit, so here we go.
>>>
>>> Your intuition was right, there is a problem with this patch. In some place, like at least themes/tomahawk/template/AppBarClose.ftl the CSRF token
>>> is not generated. Because in those places OfbizUrlTransform is used.
>>>
>>> An immediate solution is to add the CSRF token generation in OfbizUrlTransform class. I just did so at OFBIZ-11317
>>>
>>> But I agree that it's not satisfactory and your proposition to deprecate OfbizUrlTransform in favour of UrlRegexpTransform makes sense to me.
>>> That's mostly why I asked you a second time.
>>>
>>> Let's go a bit in the past. With  OFBIZ-5312, which was a major change started in 2013, we notably introduced UrlRegexpTransform.
>>> We used a Svn feature branch for that: http://svn.apache.org/viewvc?view=revision&revision=r1535432
>>>
>>> It's unrelated, but while at it since it was quite unfortunate, a conflict between Anil and Paul Piper (I was then working with Paul) started and
>>> I had to find a solution which ended with ecomseo.
>>> Fortunately OFBiz is flexible enough and eventually ecomseo is now a simple clone of the ecommerce webapp (like exampleext is for the example
>>> webapp). Everyone can pick her/his preferred one :).
>>>
>>> Now about OfbizUrlTransform vs UrlRegexpTransform: after OFBIZ-5312 UrlRegexpTransform was the only one used for ofbizUrl macro.
>>> That stopped for good reason (Framework dependency on product component) in 2018 with Deepak's work on OFBIZ-10463.
>>> It also shows that during this period (3 years) nobody encountered and issue with UrlRegexpTransform used to generate ofbizUrl macro.
>>>
>>> So yes, we should definitely deprecate OfbizUrlTransform in favour of UrlRegexpTransform. But before that we need to be sure that
>>> UrlRegexpTransform would not miss a feature of OfbizUrlTransform.
>>> With OFBIZ-11229, Nicolas already started to merge UrlRegexpTransform and OfbizUrlTransform classes. This issue is still open and we should use it
>>> to reach our goal.
>>> Maybe it's already OK and we have just to check. For that I'll soon attach a diff of the 2 classes at OFBIZ-11229
>>>
>>> Thanks to care!
>>>
>>> I wish a nice weekend to all of you.
>>>
>>> Jacques
>>>
>>> Le 14/02/2020 à 14:47, Michael Brohl a écrit :
>>>> Please have a closer look at your commit, the controlPath functionality is implemented in the UrlRegexpTransform class...
>>>>
>>>> The implementing class for ofbizUrl is configured in the freemarkerTransforms.properties which is OfbizurlTransform for framework/webapp but
>>>> UrlRegexpTransform for applications/product.
>>>>
>>>> This seems inconsistent to me.
>>>>
>>>> Thanks,
>>>>
>>>> Michael Brohl
>>>>
>>>> ecomify GmbH - www.ecomify.de
>>>>
>>>>
>>>> Am 14.02.20 um 14:24 schrieb Jacques Le Roux:
>>>>> Le 13/02/2020 à 13:06, Michael Brohl a écrit :
>>>>>> There's currently only few questions left for me: we have one configuration which uses org.apache.ofbiz.webapp.ftl.OfbizUrlTransform for
>>>>>> ofbizUrl and this would not support the controlPath configuration if I don't miss something. Wouldn't it be better to change this to
>>>>>> UrlRegexpTransform and deprecate OfbizUrlTransform?
>>>>>>
>>>>>> UrlRegexpTransform is located inside the product component but the macro is used in several other components as well. I think it belongs to the
>>>>>> framework/webapp instead of applications/product. What do you think?
>>>>>
>>>>> Hi Michael,
>>>>>
>>>>> I'm not sure to understand. Are you speaking about OFBiz OOTB?
>>>>>
>>>>> Because the changes in OFBIZ-11317 are only related to ofbizUrl, hence org.apache.ofbiz.webapp.ftl.OfbizUrlTransform, so of course ofbizUrl
>>>>> supports the controlPath configuration. Also why would you want to replace OfbizUrlTransform by UrlRegexpTransform?
>>>>>
>>>>> Jacques
>>>>>
>>>>
>>