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
