Re: [Zope-CMF] SVN: Products.CMFCore/branches/2.2/Products/CMFCore/exportimport/content.py remove type check that seem useless
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
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
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
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
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
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
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
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
-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
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