Re: svn commit: r989475 - in /ofbiz/trunk/framework: entity/src/org/ofbiz/entity/finder/EntityFinderUtil.java minilang/dtd/simple-methods.xsd widget/dtd/widget-form.xsd widget/dtd/widget-screen.xsd

[Scott Gray-2]

Hi Erwan,

Why the unnecessary changes?  All that was needed to change was fix the ConditionObject constructor.  It should have been checking for "field" and then falling back to "field-name" (for backwards compatibility reasons), there was no need to change the schemas and I think the previous attribute name was more consistent.

And I definitely don't agree with backporting unnecessary schema changes to 10.04.

Regards
Scott

HotWax Media
http://www.hotwaxmedia.com

On 26/08/2010, at 7:07 PM, [hidden email] wrote:

> Author: erwan
> Date: Thu Aug 26 07:07:12 2010
> New Revision: 989475
>
> URL: http://svn.apache.org/viewvc?rev=989475&view=rev
> Log:
> when using an entity-condition, the object's attribute name used in condition object parameter was not taken into account
>
> Modified:
>    ofbiz/trunk/framework/entity/src/org/ofbiz/entity/finder/EntityFinderUtil.java
>    ofbiz/trunk/framework/minilang/dtd/simple-methods.xsd
>    ofbiz/trunk/framework/widget/dtd/widget-form.xsd
>    ofbiz/trunk/framework/widget/dtd/widget-screen.xsd
>
> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/finder/EntityFinderUtil.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/finder/EntityFinderUtil.java?rev=989475&r1=989474&r2=989475&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/finder/EntityFinderUtil.java (original)
> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/finder/EntityFinderUtil.java Thu Aug 26 07:07:12 2010
> @@ -312,10 +312,10 @@ public class EntityFinderUtil {
>         protected FlexibleMapAccessor<Object> fieldNameAcsr;
>
>         public ConditionObject(Element conditionExprElement) {
> -            this.fieldNameAcsr = FlexibleMapAccessor.getInstance(conditionExprElement.getAttribute("field-name"));
> +            this.fieldNameAcsr = FlexibleMapAccessor.getInstance(conditionExprElement.getAttribute("object-name"));
>             if (this.fieldNameAcsr.isEmpty()) {
>                 // no "field-name"? try "name"
> -                this.fieldNameAcsr = FlexibleMapAccessor.getInstance(conditionExprElement.getAttribute("name"));
> +                this.fieldNameAcsr = FlexibleMapAccessor.getInstance(conditionExprElement.getAttribute("field-name"));
>             }
>         }
>
>
> Modified: ofbiz/trunk/framework/minilang/dtd/simple-methods.xsd
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/minilang/dtd/simple-methods.xsd?rev=989475&r1=989474&r2=989475&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/minilang/dtd/simple-methods.xsd (original)
> +++ ofbiz/trunk/framework/minilang/dtd/simple-methods.xsd Thu Aug 26 07:07:12 2010
> @@ -2458,7 +2458,7 @@ under the License.
>         </xs:complexType>
>     </xs:element>
>     <xs:attributeGroup name="attlist.condition-object">
> -        <xs:attribute name="field" type="xs:string" use="required">
> +        <xs:attribute name="object-name" type="xs:string" use="required">
>             <xs:annotation>
>                 <xs:documentation>
>                     Field in the current context where that condition object is that implements the entity condition interface.
>
> Modified: ofbiz/trunk/framework/widget/dtd/widget-form.xsd
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/widget/dtd/widget-form.xsd?rev=989475&r1=989474&r2=989475&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/widget/dtd/widget-form.xsd (original)
> +++ ofbiz/trunk/framework/widget/dtd/widget-form.xsd Thu Aug 26 07:07:12 2010
> @@ -1890,7 +1890,7 @@ under the License.
>         </xs:complexType>
>     </xs:element>
>     <xs:attributeGroup name="attlist.condition-object">
> -        <xs:attribute type="xs:string" name="field" use="required"/>
> +        <xs:attribute type="xs:string" name="object-name" use="required"/>
>     </xs:attributeGroup>
>     <xs:element name="field-map">
>         <xs:complexType>
>
> Modified: ofbiz/trunk/framework/widget/dtd/widget-screen.xsd
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/widget/dtd/widget-screen.xsd?rev=989475&r1=989474&r2=989475&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/widget/dtd/widget-screen.xsd (original)
> +++ ofbiz/trunk/framework/widget/dtd/widget-screen.xsd Thu Aug 26 07:07:12 2010
> @@ -699,7 +699,7 @@ under the License.
>         </xs:complexType>
>     </xs:element>
>     <xs:attributeGroup name="attlist.condition-object">
> -        <xs:attribute type="xs:string" name="field" use="required"/>
> +        <xs:attribute type="xs:string" name="object-name" use="required"/>
>     </xs:attributeGroup>
>     <xs:element name="select-field">
>         <xs:complexType>
>
>

[Erwan de FERRIERES]

Le 26/08/2010 10:19, Scott Gray a écrit :

> Hi Erwan,
>
> Why the unnecessary changes?  All that was needed to change was fix the ConditionObject constructor.  It should have been checking for "field" and then falling back to "field-name" (for backwards compatibility reasons), there was no need to change the schemas and I think the previous attribute name was more consistent.
>
> And I definitely don't agree with backporting unnecessary schema changes to 10.04.
>
> Regards
> Scott
>
> HotWax Media
> http://www.hotwaxmedia.com
>

Hi Scott,

no problem for reverting on the 10.04, it's on its way. But for trunk, I
really think the new names are more consistent than the previous ones.
Once again, if this is not ok with you, then I will look at another way
to make this work.

Cheers,
--
Erwan de FERRIERES
www.nereide.biz

[Scott Gray-2]

On 26/08/2010, at 9:26 PM, Erwan de FERRIERES wrote:

> Le 26/08/2010 10:19, Scott Gray a écrit :
>> Hi Erwan,
>>
>> Why the unnecessary changes?  All that was needed to change was fix the ConditionObject constructor.  It should have been checking for "field" and then falling back to "field-name" (for backwards compatibility reasons), there was no need to change the schemas and I think the previous attribute name was more consistent.
>>
>> And I definitely don't agree with backporting unnecessary schema changes to 10.04.
>>
>> Regards
>> Scott
>>
>> HotWax Media
>> http://www.hotwaxmedia.com
>>
> Hi Scott,
>
> no problem for reverting on the 10.04, it's on its way. But for trunk, I really think the new names are more consistent than the previous ones. Once again, if this is not ok with you, then I will look at another way to make this work.
>
> Cheers,
> --
> Erwan de FERRIERES
> www.nereide.biz

Thanks Erwan,

About the attribute name, there was effort put in not so long ago (maybe 2 years?) to get the attribute names more consistent.  One of those efforts was to rename all attributes that expect a context field name to "field".
This attribute was changed at that time but a bug was introduced because the underlying code wasn't updated to reflect the change.  What is it about "object-name" that you feel is more consistent than the "field" attribute which is used heavily?

In the simple-methods schema there are currently no attributes called "object-name", although there are two named "event-request-object-name" and "event-response-object-name" (I'm not sure what they do).
For the "field", we have 35 attribute definitions.

Regards
Scott

[David E. Jones-2]

On Aug 26, 2010, at 3:26 AM, Erwan de FERRIERES wrote:

> Le 26/08/2010 10:19, Scott Gray a écrit :
>> Hi Erwan,
>>
>> Why the unnecessary changes?  All that was needed to change was fix the ConditionObject constructor.  It should have been checking for "field" and then falling back to "field-name" (for backwards compatibility reasons), there was no need to change the schemas and I think the previous attribute name was more consistent.
>>
>> And I definitely don't agree with backporting unnecessary schema changes to 10.04.
>>
>> Regards
>> Scott
>>
>> HotWax Media
>> http://www.hotwaxmedia.com
>>
> Hi Scott,
>
> no problem for reverting on the 10.04, it's on its way. But for trunk, I really think the new names are more consistent than the previous ones. Once again, if this is not ok with you, then I will look at another way to make this work.

Yes, please do revert the attribute changes, even in the trunk. There is a big backwards compatibility issue with things like this and even if we did decide to change these attribute names (which I don't think we should, there is no precedent for attribute names like these and therefore no consistency with attributes on other tags), then the Java code should accept both names so that existing code does not break. This is what I did before with the simple-method stuff when making the attribute names more consistent, and is vital when attribute names are changed.

Either way, this should be discussed on the dev mailing list first. It's kind of an unpleasant surprise to have things changed like this for those who are using them, and we need a much better reason than something along the lines of "I like this other name better". If other committers were making that sort of change for things you are using, it would be pretty annoying wouldn't it?

-David

[Erwan de FERRIERES]

Le 26/08/2010 16:41, David E Jones a écrit :
> Yes, please do revert the attribute changes, even in the trunk. There is a big backwards compatibility issue with things like this and even if we did decide to change these attribute names (which I don't think we should, there is no precedent for attribute names like these and therefore no consistency with attributes on other tags), then the Java code should accept both names so that existing code does not break. This is what I did before with the simple-method stuff when making the attribute names more consistent, and is vital when attribute names are changed.
>
> Either way, this should be discussed on the dev mailing list first. It's kind of an unpleasant surprise to have things changed like this for those who are using them, and we need a much better reason than something along the lines of "I like this other name better". If other committers were making that sort of change for things you are using, it would be pretty annoying wouldn't it?
>
> -David

Sorry guys...
It has just been reverted at 989772.

Regards,

--
Erwan de FERRIERES
www.nereide.biz

[BJ Freeman]

are you willing to do a migration for the new names.
remember there are production servers and making such changes effect a
customer base.
I really think that support of migration should be enforced for all
changes. I also think that would slow down arbitrary changes against the
design.
though I don't have a vote I am against such changes.

=========================
BJ Freeman  <http://bjfreeman.elance.com>
Strategic Power Office with Supplier Automation  <http://www.businessesnetwork.com/automation/viewforum.php?f=52>
Specialtymarket.com  <http://www.specialtymarket.com/>
Systems Integrator-- Glad to Assist

Chat  Y! messenger: bjfr33man


Erwan de FERRIERES sent the following on 8/26/2010 2:26 AM:

> Le 26/08/2010 10:19, Scott Gray a écrit :
>> Hi Erwan,
>>
>> Why the unnecessary changes? All that was needed to change was fix the
>> ConditionObject constructor. It should have been checking for "field"
>> and then falling back to "field-name" (for backwards compatibility
>> reasons), there was no need to change the schemas and I think the
>> previous attribute name was more consistent.
>>
>> And I definitely don't agree with backporting unnecessary schema
>> changes to 10.04.
>>
>> Regards
>> Scott
>>
>> HotWax Media
>> http://www.hotwaxmedia.com
>>
> Hi Scott,
>
> no problem for reverting on the 10.04, it's on its way. But for trunk, I
> really think the new names are more consistent than the previous ones.
> Once again, if this is not ok with you, then I will look at another way
> to make this work.
>
> Cheers,

[Jacques Le Roux]

BJ,

I guess we were all against, anyway Erwan gently reverted it at 989772

Jacques

From: "BJ Freeman" <[hidden email]>

> are you willing to do a migration for the new names.
> remember there are production servers and making such changes effect a customer base.
> I really think that support of migration should be enforced for all changes. I also think that would slow down arbitrary changes
> against the design.
> though I don't have a vote I am against such changes.
>
> =========================
> BJ Freeman  <http://bjfreeman.elance.com>
> Strategic Power Office with Supplier Automation  <http://www.businessesnetwork.com/automation/viewforum.php?f=52>
> Specialtymarket.com  <http://www.specialtymarket.com/>
> Systems Integrator-- Glad to Assist
>
> Chat  Y! messenger: bjfr33man
>
>
> Erwan de FERRIERES sent the following on 8/26/2010 2:26 AM:
>> Le 26/08/2010 10:19, Scott Gray a écrit :
>>> Hi Erwan,
>>>
>>> Why the unnecessary changes? All that was needed to change was fix the
>>> ConditionObject constructor. It should have been checking for "field"
>>> and then falling back to "field-name" (for backwards compatibility
>>> reasons), there was no need to change the schemas and I think the
>>> previous attribute name was more consistent.
>>>
>>> And I definitely don't agree with backporting unnecessary schema
>>> changes to 10.04.
>>>
>>> Regards
>>> Scott
>>>
>>> HotWax Media
>>> http://www.hotwaxmedia.com
>>>
>> Hi Scott,
>>
>> no problem for reverting on the 10.04, it's on its way. But for trunk, I
>> really think the new names are more consistent than the previous ones.
>> Once again, if this is not ok with you, then I will look at another way
>> to make this work.
>>
>> Cheers,
>

[BJ Freeman]

yes Davids and Erwans emails came in after i posted. my email client
does not always pull from the server in time for me to see other replies
sorry about that.

=========================
BJ Freeman  <http://bjfreeman.elance.com>
Strategic Power Office with Supplier Automation  <http://www.businessesnetwork.com/automation/viewforum.php?f=52>
Specialtymarket.com  <http://www.specialtymarket.com/>
Systems Integrator-- Glad to Assist

Chat  Y! messenger: bjfr33man


Jacques Le Roux sent the following on 8/26/2010 10:27 AM:

> BJ,
>
> I guess we were all against, anyway Erwan gently reverted it at 989772
>
> Jacques
>
> From: "BJ Freeman" <[hidden email]>
>> are you willing to do a migration for the new names.
>> remember there are production servers and making such changes effect a
>> customer base.
>> I really think that support of migration should be enforced for all
>> changes. I also think that would slow down arbitrary changes against
>> the design.
>> though I don't have a vote I am against such changes.
>>
>> =========================
>> BJ Freeman <http://bjfreeman.elance.com>
>> Strategic Power Office with Supplier Automation
>> <http://www.businessesnetwork.com/automation/viewforum.php?f=52>
>> Specialtymarket.com <http://www.specialtymarket.com/>
>> Systems Integrator-- Glad to Assist
>>
>> Chat Y! messenger: bjfr33man
>>
>>
>> Erwan de FERRIERES sent the following on 8/26/2010 2:26 AM:
>>> Le 26/08/2010 10:19, Scott Gray a écrit :
>>>> Hi Erwan,
>>>>
>>>> Why the unnecessary changes? All that was needed to change was fix the
>>>> ConditionObject constructor. It should have been checking for "field"
>>>> and then falling back to "field-name" (for backwards compatibility
>>>> reasons), there was no need to change the schemas and I think the
>>>> previous attribute name was more consistent.
>>>>
>>>> And I definitely don't agree with backporting unnecessary schema
>>>> changes to 10.04.
>>>>
>>>> Regards
>>>> Scott
>>>>
>>>> HotWax Media
>>>> http://www.hotwaxmedia.com
>>>>
>>> Hi Scott,
>>>
>>> no problem for reverting on the 10.04, it's on its way. But for trunk, I
>>> really think the new names are more consistent than the previous ones.
>>> Once again, if this is not ok with you, then I will look at another way
>>> to make this work.
>>>
>>> Cheers,
>>
>
>
>

[Scott Gray-2]

Hi Erwan,

Since you reverted fully but there was still a bug in the original code I've gone ahead and fixed it in r990347.

Thanks
Scott

On 27/08/2010, at 3:46 AM, Erwan de FERRIERES wrote:

> Le 26/08/2010 16:41, David E Jones a écrit :
>> Yes, please do revert the attribute changes, even in the trunk. There is a big backwards compatibility issue with things like this and even if we did decide to change these attribute names (which I don't think we should, there is no precedent for attribute names like these and therefore no consistency with attributes on other tags), then the Java code should accept both names so that existing code does not break. This is what I did before with the simple-method stuff when making the attribute names more consistent, and is vital when attribute names are changed.
>>
>> Either way, this should be discussed on the dev mailing list first. It's kind of an unpleasant surprise to have things changed like this for those who are using them, and we need a much better reason than something along the lines of "I like this other name better". If other committers were making that sort of change for things you are using, it would be pretty annoying wouldn't it?
>>
>> -David
>
> Sorry guys...
> It has just been reverted at 989772.
>
> Regards,
>
> --
> Erwan de FERRIERES
> www.nereide.biz

[Erwan de FERRIERES]

Le 28/08/2010 13:37, Scott Gray a écrit :
> Hi Erwan,
>
> Since you reverted fully but there was still a bug in the original code I've gone ahead and fixed it in r990347.
>
> Thanks
> Scott

Hi Scott,

Thanks for doing it !

cheers,

--
Erwan de FERRIERES
www.nereide.biz