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
