Adrian Crum wrote: > Adam Heath wrote: >> Adrian Crum wrote: >>> Adam Heath wrote: >>>> Adrian Crum wrote: >>>>> doo...@apache.org wrote: >>>>>> Author: doogie >>>>>> Date: Mon Mar 1 05:06:16 2010 >>>>>> New Revision: 917376 >>>>>> >>>>>> URL: http://svn.apache.org/viewvc?rev=917376&view=rev >>>>>> Log: >>>>>> BUG FIX: Move the check that tests whether we are converting to the >>>>>> passed object to before the loadClass call. >>>>>> >>>>>> Modified: >>>>>> >>>>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java >>>>>> >>>>>> Modified: >>>>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java >>>>>> URL: >>>>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java?rev=917376&r1=917375&r2=917376&view=diff >>>>>> >>>>>> >>>>>> >>>>>> ============================================================================== >>>>>> >>>>>> >>>>>> >>>>>> --- >>>>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java >>>>>> (original) >>>>>> +++ >>>>>> ofbiz/trunk/framework/base/src/org/ofbiz/base/util/ObjectType.java >>>>>> Mon Mar 1 05:06:16 2010 >>>>>> @@ -484,6 +484,9 @@ >>>>>> return obj.toString(); >>>>>> } >>>>>> Class<?> sourceClass = obj.getClass(); >>>>>> + if (sourceClass.getName().equals(type)) { >>>>>> + return obj; >>>>>> + } >>>>>> if ("Object".equals(type) || >>>>>> "java.lang.Object".equals(type)) { >>>>>> return obj; >>>>>> } >>>>> Are you sure this will work? Take a look at the following if >>>>> statement. >>>> Yes, due a combination of things. >>>> >>>> If the object was null, return null. >>>> >>>> If converting to the same exact class as the incoming obj, return with >>>> no change. >>>> >>>> If the incoming object is a string, and it is empty, return null. >>>> >>>> The above 3 constraints were pulled from the old version of >>>> simpleTypeConvert. >>>> >>>> Without all three, if you tried to convert an empty String to a >>>> String, you'd get null. >>> I think you missed my point. Shouldn't the test be something like: >>> >>> if (sourceClass.getName().equals(type) || >>> sourceClass.getSimpleName().equals(type)) { >>> return obj; >>> } >>> >>> ? >>> >>> In other words, you can't count on type being "java.util.Date" - it >>> might be simply "Date". >> >> Except always testing against simpleName isn't right, as only certain >> classes had simpleName tests from the previous version. >> >> I see your point, however, and will fix it how you want, but only >> insomuch that it matches the old version. > > I didn't care for the simple name test in the original code. Maybe > that's why I changed it to be more reliable. > > Like you said in your summary, there were things in the original code > that didn't work or didn't make sense.
Ok, no, I will not be implementing the change you suggest. The original code did not check the short name there. And yes, that is weird, but this series of fixes was to only make the new version function exactly like the old, so that existing code wouldn't break. Now, if we want to discuss removing these extra changes, then we also need to take it upon ourselves to fix any uses that required the code to function in this manner. >
