plone / plone.app.blocks

A 'blocks' rendering model for Plone
https://pypi.python.org/pypi/plone.app.blocks
Other
11 stars 4 forks source link

Should blocks' transforms be opt-in instead of opt-out #11

Closed datakurre closed 10 years ago

datakurre commented 10 years ago

There has been a long discussion about plone.app.blocks causing issues with AJAX HTML-snippets and JSON-data (with wrong content-type header).

https://github.com/plone/plone.app.blocks/issues/5

What do you think, should be make the transform opt-in instead of opt-out? @bloodbare @simahawk @gyst @hvelarde @robgietema

The change would be that plone.app.blocks would not register its IBlocksLayer during GS import (install/activation), but each view that require transform, should apply it to the request individually.

In c.cover that would require either the view of c.cover doing alsoProvides for request or just to register the layer in its own GS profile.

In Plone Mosaic that would require alsoProvies into the default view of ILayoutAware (in p.a.blocks' mosaicsprint-branch) and the same in main_template-view and content-type view traversers in p.a.mosaic.

In c.dynamicmosaic this would require something else (I've not yet looked into its implementation).

There might be some pro's for this, e.g. you could AJAX load site layouts and content layouts without getting them tile-transformed automatically.

gyst commented 10 years ago

On 17/06/14 08:18, Asko Soukka wrote:

There has been a long discussion about plone.app.blocks causing issues with AJAX HTML-snippets and JSON-data (with wrong content-type header).

5 https://github.com/plone/plone.app.blocks/issues/5

What do you think, should be make the transform opt-in instead of opt-out? @bloodbare https://github.com/bloodbare @simahawk https://github.com/simahawk @gyst https://github.com/gyst @hvelarde https://github.com/hvelarde @robgietema https://github.com/robgietema

The change would be that plone.app.blocks would not register its IBlocksLayer during GS import (install/activation), but each view that require transform, should apply it to the request individually.

That's ugly.

Ideally you'd want the blocks transform to behave as a multi-adapter on (view, request) so you could switch it on/off via a marker interface on the view.

However, the transform chain is coded in a way that only looks at the request, agnostic of the view. To then implement per-view behavioral switching by toggling an interface on the request feels icky.

Not that I have a better alternative :-(

If we were to go that route, I propose to keep IBlocksLayer on by default since that conforms to what everybody is expecting from other packages, and it is also useful by itself (e.g. the resource views ought to be bound to that layer).

Instead use a separate marker IBlocksTransformEnabled to switch on the transform adapters.

In c.cover that would require either the view of c.cover doing alsoProvides for request or just to register the layer in its own GS profile.

GS would not be valid, since it would apply globally across the whole site and might break other add-ons on non-cover pages in the site.

In Plone Mosaic that would require alsoProvies into the default view of ILayoutAware (in p.a.blocks' mosaicsprint-branch) and the same in main_template-view and content-type view traversers in p.a.mosaic.

In c.dynamicmosaic this would require something else (I've not yet looked into its implementation).

Extends plone.app.blocks, so same change applying alsoProvides(IBlocksTransformEnabled) on the request for each view.

There might be some pro's for this, e.g. you could AJAX load site layouts and content layouts without getting them tile-transformed automatically.

:*CU

 Guido Stevens  |  +31.43.3618933  |  http://cosent.nl

 s o c i a l   k n o w l e d g e   t e c h n o l o g y
datakurre commented 10 years ago

Guido Stevens wrote:

The change would be that plone.app.blocks would not register its IBlocksLayer during GS import (install/activation), but each view that require transform, should apply it to the request individually.

Not that I have a better alternative :-(

If we were to go that route, I propose to keep IBlocksLayer on by default since that conforms to what everybody is expecting from other packages, and it is also useful by itself (e.g. the resource views ought to be bound to that layer).

Instead use a separate marker IBlocksTransformEnabled to switch on the transform adapters.

Re-implementing plone.transformchain is not an option, so this is the only reasonable way (it's also nice that adding a new marker does alter other possible IBlockLayer usage).

If we'd like to make blocks transforms opt-in, this would be the way.

In c.cover that would require either the view of c.cover doing alsoProvides for request or just to register the layer in its own GS profile.

GS would not be valid, since it would apply globally across the whole site and might break other add-ons on non-cover pages in the site.

Agreed, but it's already doing just that. Of course, your suggestion would fix the current situation.

gyst commented 10 years ago

Actually I just checked, the transform chain does have access to the view. It already is a multi adapter and the first argument, published, is the actual BrowserView object in the scenarios we want to cover (it can also be some layout resource but that doesn't matter).

I propose to create a new interface IBlocksTransformEnabled and register the transform adapters for IBlocksTransformEnabled IBlocksLayer i.e. as a multi adapter on both view and request.

View implementations then would have to provide IBlocksTransformEnabled for the transformation chain to kick in. That solves the ajax and json stuff. It does require add-ons to add the new interface on their custom view classes.

All in all implements(IBlocksTransformEnabled) on the view class is much more elegant than requiring the view class to mark that interface on the request.

datakurre commented 10 years ago

@gyst Thanks for figuring this out. I'll try this soon, if that works, and if so, fix this for mosaicsprint-branch.

gyst commented 10 years ago

On 25-06-14 10:19, Asko Soukka wrote:

@gyst https://github.com/gyst Thanks for figuring this out. I'll try this soon, if that works, and if so, fix this for mosaicsprint-branch.

@datakurre I started working on this in a feature branch but got more test failures than I bargained for - didn't save the work. But it does require revisiting all the tests.

Additionally, it's not necessary to make all the transforms keyed on the specific view interface. It's probably enough to only bind the XML serialization transform that specifically to the marker interface. All the other transforms already have abortion clauses if the input is not XML serialized. And indeed, it's the XML serialization specifically that causes the json issues.

:*XU

 Guido Stevens  |  +31.43.3618933  |  http://cosent.nl

 s o c i a l   k n o w l e d g e   t e c h n o l o g y
datakurre commented 10 years ago

Implemented in https://github.com/plone/plone.app.blocks/pull/12