Re: [Zope-CMF] SVN: Products.CMFCore/branches/2.2/Products/CMFCore/exportimport/content.py remove type check that seem useless

2011-01-13 Thread Godefroid Chapelle
Hi,

I have fixed an exportimport bug on branch 2.2 : see tests in revision 
119560.

I'd like some review before merging the fix into trunk.

I do not know for sure that I can remove the type check.

Thanks

Gotcha

Le 13/01/11 12:00, Godefroid Chapelle a écrit :
 Log message for revision 119561:
remove type check that seem useless

 Changed:
U   Products.CMFCore/branches/2.2/Products/CMFCore/exportimport/content.py

 -=-
 Modified: 
 Products.CMFCore/branches/2.2/Products/CMFCore/exportimport/content.py
 ===
 --- Products.CMFCore/branches/2.2/Products/CMFCore/exportimport/content.py
 2011-01-13 10:44:41 UTC (rev 119560)
 +++ Products.CMFCore/branches/2.2/Products/CMFCore/exportimport/content.py
 2011-01-13 11:00:16 UTC (rev 119561)
 @@ -101,15 +101,9 @@
   parser = ConfigParser()

   title = self.context.Title()
 -if isinstance(title, unicode):
 -title_str = title.encode(self._encoding)
 -else:
 -title_str = title
 +title_str = title.encode(self._encoding)
   description = self.context.Description()
 -if isinstance(description, unicode):
 -description_str = description.encode(self._encoding)
 -else:
 -description_str = description
 +description_str = description.encode(self._encoding)
   parser.set('DEFAULT', 'Title', title_str)
   parser.set('DEFAULT', 'Description', description_str)


-- 
Godefroid Chapelle (aka __gotcha) http://bubblenet.be

___
Zope-CMF maillist  -  Zope-CMF@zope.org
https://mail.zope.org/mailman/listinfo/zope-cmf

See https://bugs.launchpad.net/zope-cmf/ for bug reports and feature requests


Re: [Zope-CMF] SVN: Products.CMFCore/branches/2.2/Products/CMFCore/exportimport/content.py remove type check that seem useless

2011-01-13 Thread Wichert Akkerman
On 1/13/11 12:04 , Godefroid Chapelle wrote:
 Hi,

 I have fixed an exportimport bug on branch 2.2 : see tests in revision
 119560.

Aren't you risking double encoding now? That patch looks like it makes 
things worse, not better.

Wichert.
___
Zope-CMF maillist  -  Zope-CMF@zope.org
https://mail.zope.org/mailman/listinfo/zope-cmf

See https://bugs.launchpad.net/zope-cmf/ for bug reports and feature requests


Re: [Zope-CMF] SVN: Products.CMFCore/branches/2.2/Products/CMFCore/exportimport/content.py remove type check that seem useless

2011-01-13 Thread Godefroid Chapelle
Le 13/01/11 12:07, Wichert Akkerman a écrit :
 On 1/13/11 12:04 , Godefroid Chapelle wrote:
 Hi,

 I have fixed an exportimport bug on branch 2.2 : see tests in revision
 119560.

 Aren't you risking double encoding now? That patch looks like it makes
 things worse, not better.

I am not sure I understand what you mean.

I did try the patch on one of my sites where export was raising an 
exception. Export did work and the files produced make sense to me.

In the branch, prior to apply the patch, I added tests that mimic my 
site setup and do not pass.

With the patch, they pass.


 Wichert.

-- 
Godefroid Chapelle (aka __gotcha) http://bubblenet.be

___
Zope-CMF maillist  -  Zope-CMF@zope.org
https://mail.zope.org/mailman/listinfo/zope-cmf

See https://bugs.launchpad.net/zope-cmf/ for bug reports and feature requests


Re: [Zope-CMF] SVN: Products.CMFCore/branches/2.2/Products/CMFCore/exportimport/content.py remove type check that seem useless

2011-01-13 Thread Wichert Akkerman
On 1/13/11 14:01 , Godefroid Chapelle wrote:


 Le 13/01/11 12:07, Wichert Akkerman a écrit :
 On 1/13/11 12:04 , Godefroid Chapelle wrote:
 Hi,

 I have fixed an exportimport bug on branch 2.2 : see tests in revision
 119560.

 Aren't you risking double encoding now? That patch looks like it makes
 things worse, not better.

 I am not sure I understand what you mean.

 I did try the patch on one of my sites where export was raising an
 exception. Export did work and the files produced make sense to me.

 In the branch, prior to apply the patch, I added tests that mimic my
 site setup and do not pass.

But do you know why? The original code was more robust than your 
version, so I am extremely curious why it was failing for you and is not 
now.

Wichert.
___
Zope-CMF maillist  -  Zope-CMF@zope.org
https://mail.zope.org/mailman/listinfo/zope-cmf

See https://bugs.launchpad.net/zope-cmf/ for bug reports and feature requests


Re: [Zope-CMF] SVN: Products.CMFCore/branches/2.2/Products/CMFCore/exportimport/content.py remove type check that seem useless

2011-01-13 Thread Godefroid Chapelle
Le 13/01/11 14:02, Wichert Akkerman a écrit :
 On 1/13/11 14:01 , Godefroid Chapelle wrote:


 Le 13/01/11 12:07, Wichert Akkerman a écrit :
 On 1/13/11 12:04 , Godefroid Chapelle wrote:
 Hi,

 I have fixed an exportimport bug on branch 2.2 : see tests in revision
 119560.

 Aren't you risking double encoding now? That patch looks like it makes
 things worse, not better.

 I am not sure I understand what you mean.

 I did try the patch on one of my sites where export was raising an
 exception. Export did work and the files produced make sense to me.

 In the branch, prior to apply the patch, I added tests that mimic my
 site setup and do not pass.

 But do you know why? The original code was more robust than your
 version, so I am extremely curious why it was failing for you and is not
 now.

 Wichert.

Reproducing the bug is extremely easy : install a bare Plone 4 site with 
language french (this creates content with accented characters that 
trigger the bug) then try an export or snapshot in portal_setup.

What your comment might imply is that the creation of default content in 
a Plone site is the culprit, rather than the export code in CMFCore.

You make me wonder if I am actually reproducing something wrong in my 
tests setup : am I allowed to set a unicode value in Title and Description ?

The code hereunder is setting up the tests :


 ITEMS_TITLE = u'Actualit\xe9'
 ITEMS_DESCRIPTION = u'Actualit\xe9 r\xe9centes'
 for id in ITEM_IDS:
 site._setObject(id, _makeINIAware(id))
 item = getattr(site, id)
 item.setTitle(ITEMS_TITLE)
 item.setDescription(ITEMS_DESCRIPTION)


-- 
Godefroid Chapelle (aka __gotcha) http://bubblenet.be

___
Zope-CMF maillist  -  Zope-CMF@zope.org
https://mail.zope.org/mailman/listinfo/zope-cmf

See https://bugs.launchpad.net/zope-cmf/ for bug reports and feature requests


Re: [Zope-CMF] SVN: Products.CMFCore/branches/2.2/Products/CMFCore/exportimport/content.py remove type check that seem useless

2011-01-13 Thread Godefroid Chapelle
Le 13/01/11 14:17, Godefroid Chapelle a écrit :
 Le 13/01/11 14:02, Wichert Akkerman a écrit :
 On 1/13/11 14:01 , Godefroid Chapelle wrote:


 Le 13/01/11 12:07, Wichert Akkerman a écrit :
 On 1/13/11 12:04 , Godefroid Chapelle wrote:
 Hi,

 I have fixed an exportimport bug on branch 2.2 : see tests in revision
 119560.

 Aren't you risking double encoding now? That patch looks like it makes
 things worse, not better.

 I am not sure I understand what you mean.

 I did try the patch on one of my sites where export was raising an
 exception. Export did work and the files produced make sense to me.

 In the branch, prior to apply the patch, I added tests that mimic my
 site setup and do not pass.

 But do you know why? The original code was more robust than your
 version, so I am extremely curious why it was failing for you and is not
 now.

 Wichert.

 Reproducing the bug is extremely easy : install a bare Plone 4 site with
 language french (this creates content with accented characters that
 trigger the bug) then try an export or snapshot in portal_setup.

 What your comment might imply is that the creation of default content in
 a Plone site is the culprit, rather than the export code in CMFCore.

 You make me wonder if I am actually reproducing something wrong in my
 tests setup : am I allowed to set a unicode value in Title and Description ?

 The code hereunder is setting up the tests :

 
   ITEMS_TITLE = u'Actualit\xe9'
   ITEMS_DESCRIPTION = u'Actualit\xe9 r\xe9centes'
   for id in ITEM_IDS:
   site._setObject(id, _makeINIAware(id))
   item = getattr(site, id)
   item.setTitle(ITEMS_TITLE)
   item.setDescription(ITEMS_DESCRIPTION)
 

I want to add that this setup triggers the same exception as in a live 
Plone site.

-- 
Godefroid Chapelle (aka __gotcha) http://bubblenet.be

___
Zope-CMF maillist  -  Zope-CMF@zope.org
https://mail.zope.org/mailman/listinfo/zope-cmf

See https://bugs.launchpad.net/zope-cmf/ for bug reports and feature requests


Re: [Zope-CMF] SVN: Products.CMFCore/branches/2.2/Products/CMFCore/exportimport/content.py remove type check that seem useless

2011-01-13 Thread Charlie Clark
Am 13.01.2011, 14:01 Uhr, schrieb Godefroid Chapelle got...@bubblenet.be:

 Aren't you risking double encoding now? That patch looks like it makes
 things worse, not better.
 I am not sure I understand what you mean.

Hi Godefroid,

the unpatched code checks to see whether a self.Title() or  
self.Description() are unicode in which case they can be encoded. Your  
patch will force encoding even on strings, ie. double-encoding.

Regarding the procedure: changes to the CMF should be tested on CMF  
buildouts, with CMF specific tests. You should probably run the  
exportimport with pdb to see what self.Title() is returning but, yes, I  
suspect the problem you are experiencing is Plone and not CMF related.

Charlie
-- 
Charlie Clark
Managing Director
Clark Consulting  Research
German Office
Helmholtzstr. 20
Düsseldorf
D- 40215
Tel: +49-211-600-3657
Mobile: +49-178-782-6226
___
Zope-CMF maillist  -  Zope-CMF@zope.org
https://mail.zope.org/mailman/listinfo/zope-cmf

See https://bugs.launchpad.net/zope-cmf/ for bug reports and feature requests


Re: [Zope-CMF] SVN: Products.CMFCore/branches/2.2/Products/CMFCore/exportimport/content.py remove type check that seem useless

2011-01-13 Thread Godefroid Chapelle
Le 13/01/11 14:41, Charlie Clark a écrit :
 Am 13.01.2011, 14:01 Uhr, schrieb Godefroid Chapellegot...@bubblenet.be:

 Aren't you risking double encoding now? That patch looks like it makes
 things worse, not better.
 I am not sure I understand what you mean.

 Hi Godefroid,

 the unpatched code checks to see whether a self.Title() or
 self.Description() are unicode in which case they can be encoded.

I think what you call the unpatched code is my first attempt at fixing 
the bug (iow revision 119560).

My first message to the list might not be clear enough to express that I 
am actually asking review for both revision 119560 and 119561.

 Your patch will force encoding even on strings, ie. double-encoding.

If I understand well, you tell me that my first attempt could be correct 
while the second revision takes the risk to double encode.

Tell me if you agree with patch in 119560.

 Regarding the procedure: changes to the CMF should be tested on CMF
 buildouts, with CMF specific tests.

This is what I did ;-)

I even added the missing buildout (copied from trunk - see revision 119553).

 You should probably run the
 exportimport with pdb to see what self.Title() is returning but, yes, I
 suspect the problem you are experiencing is Plone and not CMF related.

I am still not convinced.

 Charlie

-- 
Godefroid Chapelle (aka __gotcha) http://bubblenet.be

___
Zope-CMF maillist  -  Zope-CMF@zope.org
https://mail.zope.org/mailman/listinfo/zope-cmf

See https://bugs.launchpad.net/zope-cmf/ for bug reports and feature requests


Re: [Zope-CMF] SVN: Products.CMFCore/branches/2.2/Products/CMFCore/exportimport/content.py remove type check that seem useless

2011-01-13 Thread Tres Seaver
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 01/13/2011 11:03 AM, Godefroid Chapelle wrote:
 Le 13/01/11 15:04, Tres Seaver a écrit :

 This change should be reverted -- you now double-encode any
 already-encoded UTF=8 strings.  We should probably add a test for that
 condition.
 
 Change reverted, test added in 119566.

Merci beaucoup!


Tres.
- -- 
===
Tres Seaver  +1 540-429-0999  tsea...@palladion.com
Palladion Software   Excellence by Designhttp://palladion.com
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk0vJM0ACgkQ+gerLs4ltQ6lgwCgmnZqUQ2SPfIvFJBelAd6fVOV
RaUAnR11jpd4fPu2ixHYo5/N9ctOfLmV
=SrV8
-END PGP SIGNATURE-

___
Zope-CMF maillist  -  Zope-CMF@zope.org
https://mail.zope.org/mailman/listinfo/zope-cmf

See https://bugs.launchpad.net/zope-cmf/ for bug reports and feature requests


Re: [Zope-CMF] SVN: Products.CMFCore/branches/2.2/Products/CMFCore/exportimport/content.py remove type check that seem useless

2011-01-13 Thread Godefroid Chapelle
Le 13/01/11 17:03, Godefroid Chapelle a écrit :
 Le 13/01/11 15:04, Tres Seaver a écrit :

 This change should be reverted -- you now double-encode any
 already-encoded UTF=8 strings.  We should probably add a test for that
 condition.

 Change reverted, test added in 119566.


After having tuned the patch and its tests, I merged into trunk.

-- 
Godefroid Chapelle (aka __gotcha) http://bubblenet.be

___
Zope-CMF maillist  -  Zope-CMF@zope.org
https://mail.zope.org/mailman/listinfo/zope-cmf

See https://bugs.launchpad.net/zope-cmf/ for bug reports and feature requests