Jacopo, I think you are right. I will revert and test again with the suggestion you made for the generation of help links
Jacques Jacopo Cappellato wrote: > Jacques, > > I have spent a lot of time reviewing the whole history behind these changes > and also all the comments in the Jira ticket > OFBIZ-5267. > Now I think that you should revert this change. > > Here is a summary of what happened: > > 1) in rev. 1361130 I have moved the "birt" component to specialpurpose; I > have also moved all the Birt reports from the > applications to the webapp I have defined in the specialpurpose/birt > component, leveraging the ability of the framework to > override existing applications: in this way the birt component was adding to > the applications the birt reports; when you remove > the birt component then the reports disappear nicely; this design is based on > best practices and I spent a lot of time to follow > it properly when I have separated the birt component 2) then it was reported > that in the overriden webapps the contextual help > links didn't work any more; this issue is not limited to the applications > overriden by the specialpurpose/birt component but it > is a general issue caused by a wrong design in the generation of help links; > specifically, the issue is that in > > <set field="helpTopic" value="${groovy: > parameters.componentName.toUpperCase() + '_' + > requestAttributes._CURRENT_VIEW_}"/> > > the name of the help file is composed using the component name; this is a > wrong approach and could be solved, for instance, by > using the webapp name instead of the component name; > > 3) instead of fixing the help link creation (e.g., as suggested above) you > simply, in rev. 1400463, commented out the webapp > declarations in specialpurpose/birt (WRONG), canceling a relevant part of my > initial work (#1); with this change you have > disabled all Birt reports > > 4) then it was reported (in OFBIZ-5267) that a Birt report was not available > (this because of your change in #3): after a lot of > confusion (see history of comments) you wrote it was caused by my commit at > #1 and you implemented in r1518777 an ugly mechanism > where birt webapp controllers are included in the applications controllers > but with a new attribute (requiring framework > modifications) to avoid errors if the birt component is missing (WRONG) > > I am now asking you to revert all the work you did in #3 and #4; we will then > focus on improving the help system, whose design is > ugly, to make it work properly with the overriden webapp mechanism that OFBiz > supports. > > In the future, please pay more attention in making changes that increase > entropy in the project. > > Regards, > > Jacopo > > On Aug 30, 2013, at 8:17 AM, Jacopo Cappellato > <jacopo.cappell...@hotwaxmedia.com> wrote: > >> It is annoying to see that we are still adding these references (or loose >> dependencies) from applications to specialpurpose... >> it is ugly. I will try to find some time to think to a better solution. >> >> Jacopo >> >> On Aug 29, 2013, at 11:39 PM, Jacques Le Roux <jacques.le.r...@les7arts.com> >> wrote: >> >>> No problems with me, you can change if you want. Note though that, for now, >>> it's not "the missing configuration file" which >>> "will not prevent the webapp from loading" but only if the whole component >>> is absent. >>> >>> Jacques >>> >>> Adrian Crum wrote: >>>> I would prefer an attribute name like "optional" - indicating the >>>> include is optional and the missing configuration file will not prevent >>>> the webapp from loading. >>>> >>>> -Adrian >>>> >>>> On 8/29/2013 12:37 PM, jler...@apache.org wrote: >>>>> Author: jleroux >>>>> Date: Thu Aug 29 19:37:44 2013 >>>>> New Revision: 1518777 >>>>> >>>>> URL: http://svn.apache.org/r1518777 >>>>> Log: >>>>> This fixes "Net before overhead report generates an error" >>>>> https://issues.apache.org/jira/browse/OFBIZ-5267 >>>>> >>>>> This is related with r1361130, where the birt component was moved from >>>>> framework to specialpurpose. >>>>> >>>>> The fix is to create the possibility of a loose coupling from >>>>> applications controllers to specialpurpose controllers. For that >>>>> an if-present attribute is added to the include element. When used, if >>>>> the included controller does not exist, instead of an >>>>> error and a stack trace, a warning is thrown (only in English, for >>>>> developers) suggesting to checkout the component from >>>>> trunk. Indeed, in the current case, this situation should only arise in >>>>> release after 12.04 where the birt component, like >>>>> all others but the ecommerce component, had been removed. This can be >>>>> used in other cases, but for now only the component >>>>> level is covered. >>>>> >>>>> Modified: >>>>> >>>>> ofbiz/trunk/applications/accounting/webapp/accounting/WEB-INF/controller.xml >>>>> ofbiz/trunk/applications/order/webapp/ordermgr/WEB-INF/controller.xml >>>>> ofbiz/trunk/applications/product/webapp/facility/WEB-INF/controller.xml >>>>> >>>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/location/ComponentLocationResolver.java >>>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/FileUtil.java >>>>> ofbiz/trunk/framework/webapp/dtd/site-conf.xsd >>>>> >>>>> ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/ConfigXMLReader.java >>>>> ofbiz/trunk/specialpurpose/birt/ofbiz-component.xml >>>>> >>>>> ofbiz/trunk/specialpurpose/birt/webapp/accounting/WEB-INF/controller.xml >>>>> ofbiz/trunk/specialpurpose/birt/webapp/facility/WEB-INF/controller.xml >>>>> ofbiz/trunk/specialpurpose/birt/webapp/ordermgr/WEB-INF/controller.xml >>>>> >>>>> Modified: >>>>> ofbiz/trunk/applications/accounting/webapp/accounting/WEB-INF/controller.xml >>>>> URL: >>>>> http://svn.apache.org/viewvc/ofbiz/trunk/applications/accounting/webapp/accounting/WEB-INF/controller.xml?rev=1518777&r1=1518776&r2=1518777&view=diff >>>>> ============================================================================== >>>>> --- >>>>> ofbiz/trunk/applications/accounting/webapp/accounting/WEB-INF/controller.xml >>>>> (original) +++ >>>>> ofbiz/trunk/applications/accounting/webapp/accounting/WEB-INF/controller.xml >>>>> Thu Aug 29 19:37:44 2013 @@ -22,6 +22,7 @@ under >>>>> the License. >>>>> xsi:noNamespaceSchemaLocation="http://ofbiz.apache.org/dtds/site-conf.xsd"> >>>>> <include >>>>> location="component://common/webcommon/WEB-INF/common-controller.xml"/> >>>>> <include >>>>> location="component://commonext/webapp/WEB-INF/controller.xml"/> >>>>> + <include >>>>> location="component://birt/webapp/accounting/WEB-INF/controller.xml" >>>>> if-present="true"/> >>>>> <description>Accounting Manager Module Site Configuration >>>>> File</description> >>>>> >>>>> <!-- Events to run on every request before security (chains exempt) >>>>> --> >>>>> >>>>> Modified: >>>>> ofbiz/trunk/applications/order/webapp/ordermgr/WEB-INF/controller.xml >>>>> URL: >>>>> http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/webapp/ordermgr/WEB-INF/controller.xml?rev=1518777&r1=1518776&r2=1518777&view=diff >>>>> ============================================================================== >>>>> --- >>>>> ofbiz/trunk/applications/order/webapp/ordermgr/WEB-INF/controller.xml >>>>> (original) +++ >>>>> ofbiz/trunk/applications/order/webapp/ordermgr/WEB-INF/controller.xml Thu >>>>> Aug 29 19:37:44 2013 @@ -23,6 +23,7 @@ under the >>>>> License. <include >>>>> location="component://common/webcommon/WEB-INF/common-controller.xml"/> >>>>> <include >>>>> location="component://commonext/webapp/WEB-INF/controller.xml"/> >>>>> <include >>>>> location="component://content/webapp/content/WEB-INF/controller.xml"/> >>>>> + <include >>>>> location="component://birt/webapp/ordermgr/WEB-INF/controller.xml" >>>>> if-present="true"/> >>>>> <description>Order Manager Module Site Configuration >>>>> File</description> >>>>> >>>>> <!-- event handlers --> >>>>> >>>>> Modified: >>>>> ofbiz/trunk/applications/product/webapp/facility/WEB-INF/controller.xml >>>>> URL: >>>>> http://svn.apache.org/viewvc/ofbiz/trunk/applications/product/webapp/facility/WEB-INF/controller.xml?rev=1518777&r1=1518776&r2=1518777&view=diff >>>>> ============================================================================== >>>>> --- >>>>> ofbiz/trunk/applications/product/webapp/facility/WEB-INF/controller.xml >>>>> (original) +++ >>>>> ofbiz/trunk/applications/product/webapp/facility/WEB-INF/controller.xml >>>>> Thu Aug 29 19:37:44 2013 @@ -22,6 +22,8 @@ under the >>>>> License. >>>>> xsi:noNamespaceSchemaLocation="http://ofbiz.apache.org/dtds/site-conf.xsd"> >>>>> <include >>>>> location="component://common/webcommon/WEB-INF/common-controller.xml"/> >>>>> <include >>>>> location="component://commonext/webapp/WEB-INF/controller.xml"/> >>>>> + <include >>>>> location="component://birt/webapp/accounting/WEB-INF/controller.xml" >>>>> if-present="true"/> >>>>> + >>>>> <description>Facility Manager Module Site Configuration >>>>> File</description> >>>>> >>>>> <handler name="service-multi" type="request" >>>>> class="org.ofbiz.webapp.event.ServiceMultiEventHandler"/> >>>>> >>>>> Modified: >>>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/location/ComponentLocationResolver.java >>>>> URL: >>>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/location/ComponentLocationResolver.java?rev=1518777&r1=1518776&r2=1518777&view=diff >>>>> ============================================================================== >>>>> --- >>>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/location/ComponentLocationResolver.java >>>>> (original) +++ >>>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/location/ComponentLocationResolver.java >>>>> Thu Aug 29 19:37:44 2013 @@ -38,7 +38,7 >>>>> @@ public class ComponentLocationResolver i public static final >>>>> String module = ComponentLocationResolver.class.getName(); >>>>> >>>>> public URL resolveLocation(String location) throws >>>>> MalformedURLException { >>>>> - String baseLocation = getBaseLocation(location).toString(); >>>>> + String baseLocation = getBaseLocation(location, >>>>> false).toString(); >>>>> if (File.separatorChar != '/') { >>>>> baseLocation = baseLocation.replace(File.separatorChar, '/'); >>>>> } >>>>> @@ -52,7 +52,7 @@ public class ComponentLocationResolver i >>>>> } >>>>> } >>>>> >>>>> - public static StringBuilder getBaseLocation(String location) throws >>>>> MalformedURLException { >>>>> + public static StringBuilder getBaseLocation(String location, boolean >>>>> ifPresent) throws MalformedURLException { >>>>> StringBuilder baseLocation = new >>>>> StringBuilder(FlexibleLocation.stripLocationType(location)); >>>>> // componentName is between the first slash and the second >>>>> int firstSlash = baseLocation.indexOf("/"); >>>>> @@ -73,9 +73,13 @@ public class ComponentLocationResolver i >>>>> baseLocation.insert(0, rootLocation); >>>>> return baseLocation; >>>>> } catch (ComponentException e) { >>>>> - String errMsg = "Could not get root location for component >>>>> with name [" + componentName + "], error was: " + >>>>> e.toString(); >>>>> - Debug.logError(e, errMsg, module); >>>>> - throw new MalformedURLException(errMsg); >>>>> + if (!ifPresent) { >>>>> + String errMsg = "Could not get root location for >>>>> component with name [" + componentName + "], error was: " + >>>>> e.toString(); + Debug.logError(e, errMsg, module); >>>>> + throw new MalformedURLException(errMsg); >>>>> + } else { >>>>> + return null; >>>>> + } >>>>> } >>>>> - } >>>>> + } >>>>> } >>>>> >>>>> Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/util/FileUtil.java >>>>> URL: >>>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/FileUtil.java?rev=1518777&r1=1518776&r2=1518777&view=diff >>>>> ============================================================================== >>>>> --- >>>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/FileUtil.java >>>>> (original) +++ >>>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/FileUtil.java Thu Aug >>>>> 29 19:37:44 2013 @@ -38,9 +38,8 @@ import >>>>> java.util.Set; import javolution.util.FastList; >>>>> import javolution.util.FastSet; >>>>> >>>>> -import org.ofbiz.base.location.ComponentLocationResolver; >>>>> - >>>>> import org.apache.commons.io.FileUtils; >>>>> +import org.ofbiz.base.location.ComponentLocationResolver; >>>>> >>>>> /** >>>>> * File Utilities >>>>> @@ -57,7 +56,7 @@ public class FileUtil { >>>>> public static File getFile(File root, String path) { >>>>> if (path.startsWith("component://")) { >>>>> try { >>>>> - path = >>>>> ComponentLocationResolver.getBaseLocation(path).toString(); >>>>> + path = ComponentLocationResolver.getBaseLocation(path, >>>>> false).toString(); >>>>> } catch (MalformedURLException e) { >>>>> Debug.logError(e, module); >>>>> return null; >>>>> >>>>> Modified: ofbiz/trunk/framework/webapp/dtd/site-conf.xsd >>>>> URL: >>>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/webapp/dtd/site-conf.xsd?rev=1518777&r1=1518776&r2=1518777&view=diff >>>>> ============================================================================== >>>>> --- ofbiz/trunk/framework/webapp/dtd/site-conf.xsd (original) >>>>> +++ ofbiz/trunk/framework/webapp/dtd/site-conf.xsd Thu Aug 29 19:37:44 >>>>> 2013 >>>>> @@ -55,6 +55,17 @@ under the License. >>>>> </xs:element> >>>>> <xs:attributeGroup name="attlist.include"> >>>>> <xs:attribute type="xs:string" name="location" use="required"/> >>>>> + <xs:attribute type="xs:boolean" name="if-present" use="optional" >>>>> default="true"> >>>>> + <xs:annotation> >>>>> + <xs:documentation> >>>>> + Only used with component type location >>>>> (component://). This allows to introduce a loose coupling to >>>>> another component. + >>>>> + If true, if the component is absent it will be >>>>> ignored. >>>>> + A warning will be logged, to let know the component >>>>> can possibly be checked out from trunk HEAD >>>>> + (since R13.07, in releases specialpurpose components >>>>> are removed but ecommerce). >>>>> + </xs:documentation> >>>>> + </xs:annotation> >>>>> + </xs:attribute> >>>>> </xs:attributeGroup> >>>>> <xs:element name="description" type="xs:string"/> >>>>> <xs:element name="owner" type="xs:string"/> >>>>> >>>>> Modified: >>>>> ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/ConfigXMLReader.java >>>>> URL: >>>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/ConfigXMLReader.java?rev=1518777&r1=1518776&r2=1518777&view=diff >>>>> ============================================================================== >>>>> --- >>>>> ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/ConfigXMLReader.java >>>>> (original) +++ >>>>> ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/control/ConfigXMLReader.java >>>>> Thu Aug 29 19:37:44 2013 @@ -32,6 +32,7 @@ >>>>> import javolution.util.FastList; import javolution.util.FastMap; >>>>> import javolution.util.FastSet; >>>>> >>>>> +import org.ofbiz.base.location.ComponentLocationResolver; >>>>> import org.ofbiz.base.location.FlexibleLocation; >>>>> import org.ofbiz.base.metrics.Metrics; >>>>> import org.ofbiz.base.metrics.MetricsFactory; >>>>> @@ -295,7 +296,24 @@ public class ConfigXMLReader { >>>>> protected void loadIncludes(Element rootElement) { >>>>> for (Element includeElement: >>>>> UtilXml.childElementList(rootElement, "include")) { >>>>> String includeLocation = >>>>> includeElement.getAttribute("location"); >>>>> + boolean testIfPresent = >>>>> "true".equals(includeElement.getAttribute("if-present")); >>>>> if (UtilValidate.isNotEmpty(includeLocation)) { >>>>> + if (includeLocation.startsWith("component://") && >>>>> testIfPresent) { >>>>> + try { >>>>> + if (null == >>>>> ComponentLocationResolver.getBaseLocation(includeLocation, true)) { >>>>> + Debug.logWarning(includeLocation + " >>>>> does not exist." + " Since R13.07, in releases, >>>>> specialpurpose components were removed but ecommerce." + >>>>> + " If you need this >>>>> component, you might check it out from Apache OFBiz trunk HEAD >>>>> (http://svn.apache.org/repos/asf/ofbiz/trunk)." + >>>>> + " Else, you can simply neglect this warning.", module); + >>>>> continue; >>>>> + } >>>>> + } catch (MalformedURLException mue) { >>>>> + Debug.logWarning("While trying to retrieve " >>>>> + includeLocation + " an error occured (but >>>>> if-present was used, so it's maybe only a typo)." + >>>>> + " Also note that since R13.07, in >>>>> releases, specialpurpose components were removed but ecommerce." + >>>>> + " If you need this >>>>> component, you might check it out from Apache OFBiz trunk HEAD >>>>> (http://svn.apache.org/repos/asf/ofbiz/trunk)." + >>>>> + " Else, you can simply neglect this warning.", module); + >>>>> continue; >>>>> + } >>>>> + } >>>>> try { >>>>> URL urlLocation = >>>>> FlexibleLocation.resolveLocation(includeLocation); >>>>> includes.add(urlLocation); >>>>> >>>>> Modified: ofbiz/trunk/specialpurpose/birt/ofbiz-component.xml >>>>> URL: >>>>> http://svn.apache.org/viewvc/ofbiz/trunk/specialpurpose/birt/ofbiz-component.xml?rev=1518777&r1=1518776&r2=1518777&view=diff >>>>> ============================================================================== >>>>> --- >>>>> ofbiz/trunk/specialpurpose/birt/ofbiz-component.xml (original) +++ >>>>> ofbiz/trunk/specialpurpose/birt/ofbiz-component.xml Thu >>>>> Aug 29 19:37:44 2013 @@ -29,28 +29,6 @@ under the License. >>>>> <entity-resource type="data" reader-name="seed" loader="main" >>>>> location="data/OrderPortletData.xml"/> >>>>> <service-resource type="model" loader="main" >>>>> location="servicedef/services.xml"/> >>>>> >>>>> - <!-- use when reports need to be injected into applications Note: >>>>> this will break contextual help for those applications. >>>>> - <webapp name="accounting" >>>>> - title="Accounting" >>>>> - server="default-server" >>>>> - location="webapp/accounting" >>>>> - base-permission="OFBTOOLS,ACCOUNTING" >>>>> - mount-point="/accounting"/> >>>>> - <webapp name="facility" >>>>> - title="Facility" >>>>> - description="FacilityComponentDescription" >>>>> - server="default-server" >>>>> - location="webapp/facility" >>>>> - base-permission="OFBTOOLS,FACILITY" >>>>> - mount-point="/facility"/> >>>>> - <webapp name="order" >>>>> - title="Order" >>>>> - description="OrderComponentDescription" >>>>> - server="default-server" >>>>> - location="webapp/ordermgr" >>>>> - base-permission="OFBTOOLS,ORDERMGR" >>>>> - mount-point="/ordermgr"/> >>>>> - --> >>>>> <webapp name="birt" >>>>> title="BIRT" >>>>> server="default-server" >>>>> >>>>> Modified: >>>>> ofbiz/trunk/specialpurpose/birt/webapp/accounting/WEB-INF/controller.xml >>>>> URL: >>>>> http://svn.apache.org/viewvc/ofbiz/trunk/specialpurpose/birt/webapp/accounting/WEB-INF/controller.xml?rev=1518777&r1=1518776&r2=1518777&view=diff >>>>> ============================================================================== >>>>> --- >>>>> ofbiz/trunk/specialpurpose/birt/webapp/accounting/WEB-INF/controller.xml >>>>> (original) +++ >>>>> ofbiz/trunk/specialpurpose/birt/webapp/accounting/WEB-INF/controller.xml >>>>> Thu Aug 29 19:37:44 2013 @@ -20,7 +20,6 @@ under the >>>>> License. >>>>> >>>>> <site-conf xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" >>>>> >>>>> xsi:noNamespaceSchemaLocation="http://ofbiz.apache.org/dtds/site-conf.xsd"> >>>>> - <include >>>>> location="component://accounting/webapp/accounting/WEB-INF/controller.xml"/> >>>>> >>>>> <description>Extended Accounting Controller Configuration >>>>> File</description> >>>>> >>>>> >>>>> Modified: >>>>> ofbiz/trunk/specialpurpose/birt/webapp/facility/WEB-INF/controller.xml >>>>> URL: >>>>> http://svn.apache.org/viewvc/ofbiz/trunk/specialpurpose/birt/webapp/facility/WEB-INF/controller.xml?rev=1518777&r1=1518776&r2=1518777&view=diff >>>>> ============================================================================== >>>>> --- >>>>> ofbiz/trunk/specialpurpose/birt/webapp/facility/WEB-INF/controller.xml >>>>> (original) +++ >>>>> ofbiz/trunk/specialpurpose/birt/webapp/facility/WEB-INF/controller.xml >>>>> Thu Aug 29 19:37:44 2013 @@ -20,7 +20,6 @@ under the >>>>> License. >>>>> >>>>> <site-conf xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" >>>>> >>>>> xsi:noNamespaceSchemaLocation="http://ofbiz.apache.org/dtds/site-conf.xsd"> >>>>> - <include >>>>> location="component://product/webapp/facility/WEB-INF/controller.xml"/> >>>>> >>>>> <description>Extended Facility Manager Controller Configuration >>>>> File</description> >>>>> >>>>> >>>>> Modified: >>>>> ofbiz/trunk/specialpurpose/birt/webapp/ordermgr/WEB-INF/controller.xml >>>>> URL: >>>>> http://svn.apache.org/viewvc/ofbiz/trunk/specialpurpose/birt/webapp/ordermgr/WEB-INF/controller.xml?rev=1518777&r1=1518776&r2=1518777&view=diff >>>>> ============================================================================== >>>>> --- >>>>> ofbiz/trunk/specialpurpose/birt/webapp/ordermgr/WEB-INF/controller.xml >>>>> (original) +++ >>>>> ofbiz/trunk/specialpurpose/birt/webapp/ordermgr/WEB-INF/controller.xml >>>>> Thu Aug 29 19:37:44 2013 @@ -20,7 +20,6 @@ under the >>>>> License. >>>>> >>>>> <site-conf xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" >>>>> >>>>> xsi:noNamespaceSchemaLocation="http://ofbiz.apache.org/dtds/site-conf.xsd"> >>>>> - <include >>>>> location="component://order/webapp/ordermgr/WEB-INF/controller.xml"/> >>>>> >>>>> <description>Extended Order Manager Controller Configuration >>>>> File</description>