On Z2, certain imports need to come from Products.Five, to play nicely with ZPublisher and friends. I'd like to ask for the motivation for not patching it onto the existing classes and/or modules. The effect of having Z2-developers import from Products.Five is that they must opt out on packages that make use of templates, browser views, formlib, ... --- and it adds needless complexity. This split might also have prompted the Plone community to walk their own way to the extend that there isn't much code reuse outside of the core Zope packages (along with egg dependencies, but with our fake eggs, we're almost up to par here). \malthe
Malthe Borch <mborch@gmail.com> writes:
On Z2, certain imports need to come from Products.Five, to play nicely with ZPublisher and friends.
I'd like to ask for the motivation for not patching it onto the existing classes and/or modules. The effect of having Z2-developers import from Products.Five is that they must opt out on packages that make use of templates, browser views, formlib, ... --- and it adds needless complexity.
IMO, any implicitness here will eventually slap you in your face, cause bugs, and be hard to figure out. I already dislike the way that e.g. FiveFormLibMixin implicitly changes my request to be more conform with formlib . This has caused confusion and pain for me in the past. Therefore, I'd argue that we should, in contrary to what you suggest, make the Zope 2 compatibility layer more explicit in the form of utility functions, instead of more implicit. Because it makes things more transparent and easier to debug. One concrete argument against patching the Zope 3 view page template in Five to be Zope 2 conform is that this would stand in the way of legitimate use of pure Zope 3 views in Zope 2 applications, which is something that's quite feasible today. We don't to lock ourselves into a situation where Z3 templates in Z2 assume that the view is acquisition wrapped. Daniel
Daniel Nouri wrote:
Therefore, I'd argue that we should, in contrary to what you suggest, make the Zope 2 compatibility layer more explicit in the form of utility functions, instead of more implicit. Because it makes things more transparent and easier to debug.
You might be right; but it's a very bad place to be, stuck in the middle of two frameworks. We're duplicating code all over the place, and while this has obvious inherent qualities, it also wears us out as a community. I fail to understand why people are not more motivated to get rid of all the cruft. There are times when I wish the ship would sink, if only to get on with it. \malthe
Malthe Borch <mborch@gmail.com> writes:
Daniel Nouri wrote:
Therefore, I'd argue that we should, in contrary to what you suggest, make the Zope 2 compatibility layer more explicit in the form of utility functions, instead of more implicit. Because it makes things more transparent and easier to debug.
You might be right; but it's a very bad place to be, stuck in the middle of two frameworks. We're duplicating code all over the place, and while this has obvious inherent qualities, it also wears us out as a community.
I think I found a useful pattern for how to work with z3c.forms to avoid duplication. For each page that has forms, I make two views; one is the Products.Five.BrowserView view that's looked up by the publisher and is registered with ZCML. The other is my z3c.form.form.Form that does the actual work and renders my content area. The relationship between these two forms is similar to that of forms and subforms. Here is an example of how this looks like in practice: class MyForm(z3c.form.form.Form): template = Z3ViewPageTemplateFile('form.pt') fields = z3c.form.Fields(ISomething) @button.buttonAndHandler(_('Apply'), name='apply') def handle_apply(self, action): data, errors = self.extractData() # ... class PloneView(Products.Five.BrowserView): __call__ = Z2ViewPageTemplateFile('main.pt') form = MyForm def contents(self): z2.switch_on(self) return self.form(self.context.aq_inner, self.request)() Here, main.pt is the part that renders main_template and pulls in view/contents into the "content area". z2.switch_on() is a compatibility function that fiddles a bit with the Zope 2 request to make it work with z3c.form (and also calls Five's decodeInput). Of course, PloneView is generic enough to be subclassed at this point: class MyFunnyView(PloneView): form = MyFunnyForm What's the advantage of this separation? - You don't have to worry about Acquisition in your form code, which will make out the bigger part of your code. The implications of Five's BrowserView being derived from Acquisition.Explicit has often left me confused. (Death to self.context.aq_inner!) - You can reuse form code between Zope 2 and Zope 3. - Your forms can be rendered in any part of your page, like in portlets. - No subclassing of classes in z3c.form is necessary to use the forms in Zope 2. (In contrast to Five's current way of subclassing formlib.) Daniel
Malthe Borch wrote:
On Z2, certain imports need to come from Products.Five, to play nicely with ZPublisher and friends.
I'd like to ask for the motivation for not patching it onto the existing classes and/or modules.
Technically, I think that this is going to be hard. You'd need to patch in the magic acquisition base class. Acquisition is the main reason that some of the code needed to be duplicated - without the existence of acquisition wrappers, security checks are not made for view access and things just won't work. We do explicit acquisition in those bits of code, but it's still a pain and leads to confusion around self.context in views being acquisition-wrapped weirdly, breaking some expectations around aq_parent. The other argument against monkey-patching is that monkey-patching is something to be avoided if at all possible. I think it's wise to avoid it here, as it's possible. If you monkey-patched core Zope 3 classes, the chances are that you'll break Zope 3 code that assumes certain behavior. The way to get rid of many of these problems would be to get rid of the need for acquisition. Philipp started a branch long ago that allows the acquisition system to look at a __parent__ pointer if no acquisition wrapper is present. Since our views have __parent__ pointers, this should fix the issue. This branch has been lingering in an "almost ready" state for a long time now. In general, I think acquisition is one of the major remaining stumbling block in enabling quite a bit of straightforward Zope 3 code in Zope 2. Here are some more of my thoughts on this: http://faassen.n--tree.net/blog/view/weblog/2008/01/30/0 Regards, Martijn
Martijn Faassen wrote:
Technically, I think that this is going to be hard. You'd need to patch in the magic acquisition base class. Acquisition is the main reason that some of the code needed to be duplicated - without the existence of acquisition wrappers, security checks are not made for view access and things just won't work.
I think if we could finish the philikon-aq_parent branch (or whatever it's called) that makes it possible to do acquisition using __parent__ pointers, we'd be a lot closer. Hanno and Philipp know more, but I think it's reasonably close.
We do explicit acquisition in those bits of code, but it's still a pain and leads to confusion around self.context in views being acquisition-wrapped weirdly, breaking some expectations around aq_parent.
And other bizarre things sometimes.
The way to get rid of many of these problems would be to get rid of the need for acquisition. Philipp started a branch long ago that allows the acquisition system to look at a __parent__ pointer if no acquisition wrapper is present. Since our views have __parent__ pointers, this should fix the issue. This branch has been lingering in an "almost ready" state for a long time now.
Ah, great minds think alike. ;) Martin -- Author of `Professional Plone Development`, a book for developers who want to work with Plone. See http://martinaspeli.net/plone-book
Hi. Martin Aspeli wrote:
Martijn Faassen wrote:
Technically, I think that this is going to be hard. You'd need to patch in the magic acquisition base class. Acquisition is the main reason that some of the code needed to be duplicated - without the existence of acquisition wrappers, security checks are not made for view access and things just won't work.
I think if we could finish the philikon-aq_parent branch (or whatever it's called) that makes it possible to do acquisition using __parent__ pointers, we'd be a lot closer.
Hanno and Philipp know more, but I think it's reasonably close.
It is reasonably close but as stated in some threads a few month back I hit some problems which I cannot solve (not even with walking up stack frames...). As the problems only showed themselves while doing browser testing inside Plone, I guess I can at least write some unit tests for them, so someone else can actually take a look at them more easily. I'll see if I can do that during my next 10% day [1] :) Hanno [1] http://www.jarn.com/blog/the-10-plone-manifesto
Hanno Schlichting wrote: [snip]
As the problems only showed themselves while doing browser testing inside Plone, I guess I can at least write some unit tests for them, so someone else can actually take a look at them more easily.
I'll see if I can do that during my next 10% day [1] :)
Yes, it'd be cool to have some tests that demonstrate the failure. Then come back to this list and bug people until they look into this. Regards, Martijn
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Martin Aspeli wrote:
Martijn Faassen wrote:
Technically, I think that this is going to be hard. You'd need to patch in the magic acquisition base class. Acquisition is the main reason that some of the code needed to be duplicated - without the existence of acquisition wrappers, security checks are not made for view access and things just won't work.
I think if we could finish the philikon-aq_parent branch (or whatever it's called) that makes it possible to do acquisition using __parent__ pointers, we'd be a lot closer.
AFAIU, that branch doesn't provide *acquisition* via the __parent__ pointer: rather it allows the containment-based security check, which currently uses the 'inner' acquisition wrapper, to use the chain of '__parent__' pointers as an alternative when no acquisition wrapper is present. Tres. - -- =================================================================== Tres Seaver +1 540-429-0999 tseaver@palladion.com Palladion Software "Excellence by Design" http://palladion.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFIAWdo+gerLs4ltQ4RAh/WAKCku3C7UKhJTYEr69f0qUzkgH8pQQCfbRo/ W1MlgpkUjjqovn12v1lgduc= =oSDG -----END PGP SIGNATURE-----
Tres Seaver wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Martin Aspeli wrote:
Martijn Faassen wrote:
Technically, I think that this is going to be hard. You'd need to patch in the magic acquisition base class. Acquisition is the main reason that some of the code needed to be duplicated - without the existence of acquisition wrappers, security checks are not made for view access and things just won't work. I think if we could finish the philikon-aq_parent branch (or whatever it's called) that makes it possible to do acquisition using __parent__ pointers, we'd be a lot closer.
AFAIU, that branch doesn't provide *acquisition* via the __parent__ pointer:
No, it does. It makes __parent__ pointers completely equivalent to explicit acquisition wrappers.
rather it allows the containment-based security check, which currently uses the 'inner' acquisition wrapper, to use the chain of '__parent__' pointers as an alternative when no acquisition wrapper is present.
The security machinery (AccessControl) does an aq_aquire(obj, '__roles__') check. With __parent__ pointers, this will resolve properly. The branch is indeed "ready" from Zope's own point of view. Last thing I remember is that one (!) Plone tests fails in an obscure way. Then again, one failing test sometimes is enough :)
Malthe Borch wrote:
On Z2, certain imports need to come from Products.Five, to play nicely with ZPublisher and friends.
Not really. You can inherit from zope.publisher.browser.BrowserView and Five's <browser:page /> directive will magically slap Acquisition.Explicit into a newly-created subclass that will be registered as a view.
I'd like to ask for the motivation for not patching it onto the existing classes and/or modules.
Because monkey-patches are evil. And I won't accept any "buts" here. They're just evil, hard-to-impossible to test and most important of all, absolutely unfathomable for the novice developer.
The effect of having Z2-developers import from Products.Five is that they must opt out on packages that make use of templates, browser views, formlib, ... --- and it adds needless complexity.
AFAIK, Plone already monkey patches formlib so you won't even have to change those imports.
This split might also have prompted the Plone community to walk their own way to the extend that there isn't much code reuse outside of the core Zope packages (along with egg dependencies, but with our fake eggs, we're almost up to par here).
I still believe the answer is my Acquisition + __parent__ patch. As mentionen in this thread already, it's several years old now (yikes!) but should merge quite cleanly, actually. Zope's own tests pass nicely, as do the CMF's, only Plone's tests fail in one or two places in an obscure way (and I'm talking about the whole ploneout testsuite here). *IF* you'd like to be pragmatic, I'd suggest we clean up those failing Plone tests, merge the branch and be on our way. Yes, we *might* be plastering over a potential problem in the patch, but the other 9999 tests didn't seem to be affected and intensive alpha and beta testing of that new Zope version would likely confirm or refute the existence of a serious problem. To be honest, my suspicion is that Plone is using some of the Five stuff in a way that it shouldn't be, hence causing a test failure with the cleaned up Five. Hanno removed some of the Five-raping procedures in Plone already, but there might be others lurking around and causing test failures.
Philipp von Weitershausen wrote:
*IF* you'd like to be pragmatic, I'd suggest we clean up those failing Plone tests, merge the branch and be on our way. Yes, we *might* be plastering over a potential problem in the patch, but the other 9999 tests didn't seem to be affected and intensive alpha and beta testing of that new Zope version would likely confirm or refute the existence of a serious problem. To be honest, my suspicion is that Plone is using some of the Five stuff in a way that it shouldn't be, hence causing a test failure with the cleaned up Five. Hanno removed some of the Five-raping procedures in Plone already, but there might be others lurking around and causing test failures.
I'd be happy to help clean such things up on Plone trunk. I've been waiting for this branch for aaaages. :) I do remember Hanno saying he got an infite loop/segafult somewhere, which sounds more problematic. If we are pretty sure we won't get any of those, then I'd be very supportive of a merge. Martin -- Author of `Professional Plone Development`, a book for developers who want to work with Plone. See http://martinaspeli.net/plone-book
On Mon, 14 Apr 2008 10:11:11 -0700, Philipp von Weitershausen <philipp@weitershausen.de> wrote:
*IF* you'd like to be pragmatic, I'd suggest we clean up those failing Plone tests, merge the branch and be on our way.
I'm also happy to make it visible on the Plone agenda, as long as it's made clear where the error is and what we have to fix. We all want this branch to land on trunk. :) -- Alexander Limi ยท http://limi.net
Alexander Limi wrote:
On Mon, 14 Apr 2008 10:11:11 -0700, Philipp von Weitershausen <philipp@weitershausen.de> wrote:
*IF* you'd like to be pragmatic, I'd suggest we clean up those failing Plone tests, merge the branch and be on our way.
I'm also happy to make it visible on the Plone agenda, as long as it's made clear where the error is and what we have to fix. We all want this branch to land on trunk. :)
This gets back to Hanno's suggestion of writing a clean test case that demonstrates this problem. We can then get it into Zope and fix it. This should allow cleaning up the Zope 3 integration in Zope 2 significantly. Then we can start experimenting with Zope 3-based *content* in Zope 2 too. http://faassen.n--tree.net/blog/view/weblog/2008/01/30/0 Regards, Martijn
Martijn Faassen wrote:
Alexander Limi wrote:
On Mon, 14 Apr 2008 10:11:11 -0700, Philipp von Weitershausen <philipp@weitershausen.de> wrote:
*IF* you'd like to be pragmatic, I'd suggest we clean up those failing Plone tests, merge the branch and be on our way.
This gets back to Hanno's suggestion of writing a clean test case that demonstrates this problem. We can then get it into Zope and fix it. This should allow cleaning up the Zope 3 integration in Zope 2 significantly.
I kept my promise and added the simple tests for the first two issues I found while doing testing against Plone. The third issue is more complex to reproduce in a simple way and has to do with NamedTemplateAdapters and their interaction with ViewPageTemplate files. I'll see if I can put together a unit tests for it this week as well. But for those interested in this, the first two tests should be a good start. I wrote down the technical details on this from my point of view to the list back in December. You can view that post at: http://mail.zope.org/pipermail/zope-dev/2007-December/030548.html Please ignore the horrendous code in that mail which I wrote out of despair back then. Hanno
Hi again. Hanno Schlichting wrote:
I kept my promise and added the simple tests for the first two issues I found while doing testing against Plone.
I have meanwhile fixed the first trivial issue (conflicting argument called 'instance') and added a simple test for the second one. Now after a good night of sleep here is the condensed version of the problem, for those too lazy to look at the branch and the commits: All code is in Products.Five.browser. In .tests.aqlegacy.py we define two views: class LegacyTemplate(BrowserView): template = ViewPageTemplateFile('falcon.pt') def __call__(self): return self.template() class LegacyTemplateTwo(BrowserView): def __init__(self, context, request): self.__parent__ = context self.context = context self.request = request self.template = ViewPageTemplateFile('falcon.pt') def __call__(self): return self.template() Both are registered in ZCML as: <browser:page for="*" name="template" class=".aqlegacy.LegacyTemplate" permission="zope.Public" /> <browser:page for="*" name="template_two" class=".aqlegacy.LegacyTemplateTwo" permission="zope.Public" /> And in the aqlegacy_ftests.txt we call both of them via:
view = getMultiAdapter((self.folder, request), name='template') view.template <BoundPageTemplateFile of <Products.Five.metaclass.LegacyTemplate ...>> print view() <p>The falcon has taken flight</p>
view = getMultiAdapter((self.folder, request), name='template_two') view.template <Products.Five.browser.pagetemplatefile.ViewPageTemplateFile ...> print view() TypeError: __call__() takes at least 2 arguments (1 given)
Now as you can see the only difference is that one of them uses a class variable for assigning the template and the other one is using an instance variable. From what I understand some magic place in between doesn't find the template instance variable during ZCML processing as it operates on classes only and therefor doesn't turn the template into a BoundPageTemplateFile. From what I can tell the purpose of the BoundPageTemplateFile is to inject the view class into the method call of __call__, so you don't have to pass it in explicitly for each call. It does so, via some interesting code which boils down to: class BoundPageTemplate(object): def __init__(self, pt, ob): object.__setattr__(self, 'im_func', pt) object.__setattr__(self, 'im_self', ob) def __call__(self, *args, **kw): if self.im_self is None: im_self, args = args[0], args[1:] else: im_self = self.im_self return self.im_func(im_self, *args, **kw) Using an instance variable called template and calling it later on without passing in the view as the first argument doesn't work at all in Zope3. In Zope2 it did so far, as the ViewPageTemplateFile would use Acquisition to find its view. I don't have any good idea on how to handle this problem. We can probably walk up the stack frame to find the view in most cases, as the template is called in almost all cases directly from the view. But the third test failure, which I haven't written a test for yet, breaks even then. Essentially it puts in an adapter in between the view and the template where the adapter doesn't have any reference to the view anymore, so getting to the view from the template is impossible even by walking up the stack frame. This use-case is highly specialized (the code is in plone.app.form._named) but currently it works in Zope2. Ideas from people who know more about this side of Zope are still most welcome :) Hanno
Thanks for looking into this, Hanno! Here's my feedback: Hanno Schlichting wrote:
Hanno Schlichting wrote:
I kept my promise and added the simple tests for the first two issues I found while doing testing against Plone.
I have meanwhile fixed the first trivial issue (conflicting argument called 'instance')
I took a look at it. Thanks! It's too bad that we have to duplicate so much code from Zope 3 now. Therefore I would suggest that we fix up zope.app.pagetemplate properly so that it won't conflict with 'instance'. Then we can get rid of the duplication in Five. Thanks to the individual projects nowadays, it should be trivial to update zope.app.pagetemplate. [...]
Now as you can see the only difference is that one of them uses a class variable for assigning the template and the other one is using an instance variable.
ViewPageTemplateFile etc. are only meant to be used as class attributes, never as instance attributes. This statement is also true for the current, acquisition-based one from Five. In my opinion, the fact that it accidentally worked as an instance variable isn't a very strong argument for continuing to support it. To me, this is a prime example of misusing a Five component which now leads to problems when we go pure Zope3.
From what I understand some magic place in between doesn't find the template instance variable during ZCML processing as it operates on classes only and therefor doesn't turn the template into a BoundPageTemplateFile.
It doesn't happen during ZCML processing. It's a simple class descriptor, so the magic happens in ViewPageTemplateFile's __get__ which is invoked each time you do view.template (the '.' invokes __get__). I recommend reading about new-style class descriptors and properties.
Using an instance variable called template and calling it later on without passing in the view as the first argument doesn't work at all in Zope3. In Zope2 it did so far, as the ViewPageTemplateFile would use Acquisition to find its view.
Right. This led to all sorts of weird and icky problems, so thank God it's gone now.
I don't have any good idea on how to handle this problem.
Do we really have to support instance-level templates? I would still argue that if anybody's using ViewPageTemplateFile like this, they're using it wrong. I personally have no intent on supporting this. If we really have to support it, then there's a simple solution: don't use ViewPageTemplateFile for instance-level attributes, use a variant that we provide instead. We could introduce this variant in both pre-AQ-parent and post-AQ-parent Zope versions to ease compatibility.
We can probably walk up the stack frame to find the view in most cases, as the template is called in almost all cases directly from the view.
-1. Walking up stack frames for guessing stuff like this will usually destroy you.
But the third test failure, which I haven't written a test for yet, breaks even then. Essentially it puts in an adapter in between the view and the template where the adapter doesn't have any reference to the view anymore, so getting to the view from the template is impossible even by walking up the stack frame. This use-case is highly specialized (the code is in plone.app.form._named) but currently it works in Zope2.
You're probably referring to the NamedTemplate thing that zope.formlib has (and plone.app.form reimplements the stuff for Zope 2). In zope.formlib, the same __get__ technique that ViewPageTemplateFile uses is used to get a hold of the view instance when you do view.template. plone.app.form's replacement for this technique is to use acquisition to get at the view. This obviously has to go since acquisition is no longer supported for views. In fact, if I'm not mistaken, this bit of plone.app.form can entirely be ripped out. Given that plone.app.form does monkey patches (which I'm unwilling to support in any means, the original authors made potential incompatibilities their problem when they introduced them), I'm almost certain that there will never be one version of plone.app.form that will work with both the pre-AQ-parent and the post-AQ-parent Zope. You could still try, of course. At the very least, you could make a try/except clause. Either way, as I said, I'm not offering my help for this package since it contains monkey patches.
Previously Philipp von Weitershausen wrote:
ViewPageTemplateFile etc. are only meant to be used as class attributes, never as instance attributes. This statement is also true for the current, acquisition-based one from Five.
Is that documented anywhere? I can't seem to find any interface or docstring that documents that.
In my opinion, the fact that it accidentally worked as an instance variable isn't a very strong argument for continuing to support it. To me, this is a prime example of misusing a Five component which now leads to problems when we go pure Zope3.
I'ld agree if there was a docstring or interface that made that explicit. I've updated the relevant code in plone.app.portlets though since the change is harmless. Wichert. -- Wichert Akkerman <wichert@wiggy.net> It is simple to make things. http://www.wiggy.net/ It is hard to make things simple.
Wichert Akkerman wrote:
Previously Philipp von Weitershausen wrote:
ViewPageTemplateFile etc. are only meant to be used as class attributes, never as instance attributes. This statement is also true for the current, acquisition-based one from Five.
Is that documented anywhere? I can't seem to find any interface or docstring that documents that.
I suppose not, because ViewPageTemplateFile (or ZopeTwoPageTemplateFile, as it used to be called) was and still is poorly documented. Initially, it was only used internally by the ZCML directives until people started writing the view template explicitly into the view class, much like in Zope 3. So no, there isn't documentation about the Five bit. But there *is* documentation about the Zope 3 bit (my book, for instance), so my argument is mostly based on the principle of correspondence between Five and Zope 3.
In my opinion, the fact that it accidentally worked as an instance variable isn't a very strong argument for continuing to support it. To me, this is a prime example of misusing a Five component which now leads to problems when we go pure Zope3.
I'ld agree if there was a docstring or interface that made that explicit. I've updated the relevant code in plone.app.portlets though since the change is harmless.
Cool, that's great. If this is just a matter of a docstring, I'm sure that can be arranged :)
Philipp von Weitershausen wrote:
Wichert Akkerman wrote:
Previously Philipp von Weitershausen wrote:
In my opinion, the fact that it accidentally worked as an instance variable isn't a very strong argument for continuing to support it. To me, this is a prime example of misusing a Five component which now leads to problems when we go pure Zope3.
I'ld agree if there was a docstring or interface that made that explicit. I've updated the relevant code in plone.app.portlets though since the change is harmless.
Cool, that's great. If this is just a matter of a docstring, I'm sure that can be arranged :)
I added a docstring on the branch now. If we get approval for the branch merge in the current form (see new thread) I'll backport the docstring to all current active branches. Hanno
Hanno Schlichting wrote:
Martijn Faassen wrote:
Alexander Limi wrote:
On Mon, 14 Apr 2008 10:11:11 -0700, Philipp von Weitershausen <philipp@weitershausen.de> wrote:
*IF* you'd like to be pragmatic, I'd suggest we clean up those failing Plone tests, merge the branch and be on our way.
This gets back to Hanno's suggestion of writing a clean test case that demonstrates this problem. We can then get it into Zope and fix it. This should allow cleaning up the Zope 3 integration in Zope 2 significantly.
I kept my promise and added the simple tests for the first two issues I found while doing testing against Plone.
Excellent, thank you very much! I hope this will motivate people to look into this branch again. Regards, Martijn
participants (11)
-
Alexander Limi -
Chris McDonough -
Daniel Nouri -
Hanno Schlichting -
Jens Vagelpohl -
Malthe Borch -
Martijn Faassen -
Martin Aspeli -
Philipp von Weitershausen -
Tres Seaver -
Wichert Akkerman