ofbiz-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jacques Le Roux <jacques.le.r...@les7arts.com>
Subject Re: svn commit: r1789710 - /ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/Get ContentLookupList.groovy
Date Fri, 31 Mar 2017 19:17:41 GMT
Hi Michael,
Le 31/03/2017 à 20:00, Michael Brohl a écrit :
> Hi Jacques,
>
> I think this is a functional change because you not only print the exception to the error
log but you also put it in the errorMsgList. In this way, 
> the error is printed to the user interface. This might not be intended because the VIEW_SIZE
controls the number of entries in the before it is 
> paginated and is normally set to a default if missing or wrong. No need to bring this
to the UI. [1]

You are right, no need to put that in the UI. This also resolves [2] and the need to put in
the request attribute _ERROR_MESSAGE_LIST_.
Actually it's very unlikely that we ever get this issue, but it's possible in theory, so I
agree the log is enough.
So no need to revert, I'll just remove this 2nd line in the catch.

Jacques
>
> Additionally, it is not translated because the error message is hard coded which is not
acceptable IMO. [2]
>
> I would change the implementation according to the viewIndex block at the beginning (catching
the exception and setting a default value, which 
> should be read from the configuration file as it is done in other places).
>
> Looking further, there are other weeknesses, e.g. the errorMsgList is filled after it
is already put in the request attribute _ERROR_MESSAGE_LIST_.
>
> This whole file must be refactored.
>
> Please revert or change your changes at least for [1] or [2].
>
> Thanks,
>
> Michael
>
>
> Am 31.03.17 um 18:56 schrieb jleroux@apache.org:
>> Author: jleroux
>> Date: Fri Mar 31 16:56:04 2017
>> New Revision: 1789710
>>
>> URL: http://svn.apache.org/viewvc?rev=1789710&view=rev
>> Log:
>> Fixed: Fix Default or Empty Catch block in Java and Groovy files
>> (OFBIZ-8341)
>>
>> While working on OFBIZ-7759, I stumbled upon this by chance.
>> It's unlikely that this NumberFormatException is ever caught, but not a reason
>> to swallow it.
>>
>> I checked there are no other swallowed exceptions in Groovy files.
>>
>> Modified:
>> ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy
>>
>> Modified: ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy
>> URL: 
>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy?rev=1789710&r1=1789709&r2=1789710&view=diff
>> ==============================================================================
>> --- ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy
(original)
>> +++ ofbiz/ofbiz-framework/trunk/applications/content/groovyScripts/content/GetContentLookupList.groovy
Fri Mar 31 16:56:04 2017
>> @@ -69,7 +69,8 @@ context.curFindString = curFindString
>>   try {
>>       viewSize = Integer.valueOf((String)parameters.get("VIEW_SIZE")).intValue()
>>   } catch (NumberFormatException nfe) {
>> -
>> +    Debug.logError(nfe, "Caught an exception : " + nfe.toString(), "GetContentLookupList.groovy")
>> +    errMsgList.add("Entered value is non-numeric for numeric field: " + field.getName())
>>   }
>>     context.viewSize = viewSize
>>
>>
>
>


Mime
View raw message