Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IDefaultBrowserLayer or a plain interface? #89

Closed
gforcada opened this issue Aug 11, 2015 · 11 comments
Closed

IDefaultBrowserLayer or a plain interface? #89

gforcada opened this issue Aug 11, 2015 · 11 comments

Comments

@gforcada
Copy link
Sponsor Contributor

Do we really need the IDefaultBrowserLayer from zope.publisher to create a package layer?

The example shonw on plone.browserlayer shows that is just using a plain interface from zope.interface...

/me wonders

@do3cc
Copy link
Member

do3cc commented Aug 12, 2015

Yes.
If you want to override a plone core view which is bound to the IDefaultBrowserLayer (hopefully), then you would not have a clear preference, which view to load. Yours or the default view.
If your package layer is based on IDefaultBrowserLayer, your layer is more specific, thus it is guaranteed by ZCA to be prefered.

@gforcada
Copy link
Sponsor Contributor Author

Then something is wrong with bobtemplates.plone or my setup... On https://github.com/collective/collective.contentalerts I have a browser layer deriving from IDefaultBrowserLayer and as I also have a dependency on Zope2 due to OFS.SimpleItem.SimpleItem then bin/instance refuses to start due to a conflict: the traceback.

@do3cc
Copy link
Member

do3cc commented Aug 12, 2015

Both errors are unrelated to IDefaultBrowserLayer.
In both cases the reason is that you have the egg zope.publisher in there while this behavior is still provided by Zope2. the last commit seemed to have removed this already, maybe you need to rerun buildout

@do3cc
Copy link
Member

do3cc commented Sep 7, 2015

@gforcada Can this ticket be closed?

@gforcada
Copy link
Sponsor Contributor Author

gforcada commented Sep 7, 2015

I still don't see the point of using IDefaultBrowserLayer but yes, if I'm the only one that thinks a regular interface is enough.

@gforcada gforcada closed this as completed Sep 7, 2015
@do3cc
Copy link
Member

do3cc commented Sep 7, 2015

Maybe I wasn't too clear in my first comment about the reason. Zope has no way of knowing if your view for a layer with Interface as a base class is more specific than the view registered for IDefaultBrowserLayer. IOW you get undefined behavior. The order will probably depend on the alphabet, instead of class hierarchy.

IMyProduct -> Interface
IDefaultBrowserLayer -> Interface

IMyProduct -> IDefaultBrowserLayer -> Interface
IDefaultBrowserLayer -> Interface

your product is more specific, you win always

@thet
Copy link
Member

thet commented Sep 7, 2015

@do3cc So, views, which are not registered for any browser layer get implicitly registered for IDefaultBrowserLayer, not? And core components should not explicitly register any browser layer, otherwise it's not so obvious to override them with a custom layer. Otherwise, your explanation doesn't make sense to me.

The plone.app.contenttypes browser layer derives from Interface. Maybe it would make sense to change it to IDefaultBrowserLayer, so that p.a.c always wins over any core view with the same name (currently, there are only portal skin based templates with the same name. AFAIK, browser views win over portal skin templates anyways...).

Side Note: When I want to extend another package, I let my package's browser layer inherit from the other one, so this is guaranteed to be more specific.

@do3cc
Copy link
Member

do3cc commented Sep 8, 2015

Yes. Views that are not registered for any browser layer get implictly registered for IDefaultBrowserLayer:
https://github.com/zopefoundation/zope.browserpage/blob/master/src/zope/browserpage/metaconfigure.py#L93

If a package is always required for Plone, also for older versions of Plone, then it does not need to create its own browser layer. It could not set a layer in its declaration and all is fine.

If one writes an add on, the add on should use a browser layer of its own, or at least prefix view names with something unique. Else, if two third party packages provide a view blabla without defining a browser layer, your Plone Site would not start up.

If plone.app.contenttypes derives from Interface, then this is sloppy. Luckily, my diagram was incomplete. Interface is a base class of IDefaultBrowserLayer, so a view registered without defining a layer, will be more specific than plone.app.contenttypes. By using Interface as a base class, potential errors will be masked. If there is already a view with the same name for the same content type, the other view will take precedence. If plone.app.contenttypes would have used IDefaultBrowserLayer as a layer, plone would not have started up and the developer would have received a helpful error message to debug the problem.
This kind of error is one, which could have been found in an actual code review.

Browser views always loose agains skin templates.
You can see it here:
https://github.com/zopefoundation/Zope/blob/2.13.23/src/ZPublisher/BaseRequest.py#L124
The Zope publisher first tries to do an attribute Access.
The Portal root is also a subobject of the SkinnableObjectManager and this here make attribute access to skins work:
https://github.com/do3cc/zope/blob/master/Products.CMFCore/trunk/Products/CMFCore/Skinnable.py#L50
(This is a not up to date mirror of the svn repo)

It is kind of sad that this type of information is not available in any docs.
I already spent too much time to write this and verify the information, but I will create a bug to the docs to add this information. For reference. Layers need this information:
http://docs.plone.org/develop/plone/views/layers.html
And Traversing:
http://docs.plone.org/develop/plone/serving/traversing.html

Both need a rework to be spit into reference information and recipes with tips and tricks

@do3cc
Copy link
Member

do3cc commented Sep 8, 2015

Ah something I forgot. the @@viewname exists only because the attribute access is preferred.
You can (could, I broke it somehow) test this by going to demo.plone.de and create a page search
After that, the search view is only available via @@search and not view search

@thet
Copy link
Member

thet commented Sep 8, 2015

@do3cc tnx for the detailed answer! i agree, this should be documented.
i guess i also have to adapt some packages of mine to derive from IDefaultBrowserLayer.

@gforcada
Copy link
Sponsor Contributor Author

gforcada commented Sep 8, 2015

@do3cc thanks for the explanations! I will update my add-ons according then 👍

gforcada added a commit to collective/collective.contentalerts that referenced this issue Sep 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants