bluedynamics / bda.plone.shop

Generic Shop Solution for Plone
Other
20 stars 15 forks source link

context.default_page instead of container.default_page #105

Closed tobiasherp closed 6 years ago

tobiasherp commented 6 years ago

Fix for #104: Drop the aq_parent step when computing the default page. In Plone 4.3.15 and with LinguaPlone 4.2.1 installed, the former code tried to get the 'portal_languages' tool in the Zope root (instead of the Plone site root) when e.g. the @@shop_controlpanel was opened in the site root.

In any case, in this regard it looks more plausible to me to inspect the context than the container.

rnixx commented 6 years ago

This is not a fix. If the problem is that container might be zope root we need to check for IPloneSiteRoot before acquiring the parent.

tobiasherp commented 6 years ago

I disagree. Indeed the container might be Zope root (it always is when called in the Plone site root, isn't it?), then my first idea is to check whether we need the container or the context. I think we need the context, as almost always.

The method is named context_is_default_page, but it currently inspects not the context but the container. IMO, it should indeed inspect the context.

rnixx commented 6 years ago

IIRC context.default_page returns the id of the child acting as default page. And the function checks whether a context is actually the default page in it's container. Your change would therefor never work. Instead, we can just return False if IPloneSiteRoot.providedBy(context)

rnixx commented 6 years ago

ok, getMultiAdapter(to_adapt, name='default_page') actually returns the default page itself, nevertheless we need to check it on the container

rnixx commented 6 years ago

LGTM, thank you!