search

plone / Products.CMFPlone

The core of the Plone content management system
https://plone.org
GNU General Public License v2.0
254 stars 191 forks source link

Move controlpanel permissions.zcml to plone.base ? #3744

Open gforcada opened 1 year ago

gforcada commented 1 year ago

While configuring plone.app.theming with plone/meta I could remove all but one CMFPlone usages by their plone.base replacement.

The one remaining is as the title says, plone.app.theming depends on the permissions.zcml from Products.CMFPlone.controlpanel.

Should that be moved to plone.base as well? And generally speaking, should we move most plone permissions to plone.base? 🤔

🍀

davisagli commented 1 year ago

I don't know about "most" (there are plenty of Zope permissions, plus permissions that are only relevant to in-core addons) but I would agree that plone.base is a better place for permissions to be defined than CMFPlone.

gforcada commented 1 year ago

Is there a way yo actually put a deprecation warning for this? Can we define the same permission in two places? 🤔

mauritsvanrees commented 1 year ago

I don't think you can deprecate anything here. For plone.app.theming @jensens has updated your PR and created https://github.com/plone/Products.CMFPlone/pull/3747, moving the permission to plone.app.theming. Both have been merged.

jensens commented 1 year ago

Generally, I like the idea of having specific permissions in the places where they are relevant.

However, we have defined our control panels contract (interfaces) in plone.base, but they are implemented in CMFPlone. Therefore, we could argue that they are part of the definition. However, I don't believe it's necessary to move them immediately, although we may consider it in the future.

There is a special case with plone.app.multilingual (pam), which is currently a dependency indirection zombie. The best solution, in my opinion, is to move it between Plone and CMFPlone, as it depends indirectly on CMFPlone and directly on pam (OT here, needs own issue) . Pam uses permissions from CMFPlone and inherits the base control panel to extend it.

If we don't encounter any other unsolvable cases between plone.base and CMFPlone, I suggest keeping the permissions where they are currently located.

Regarding deprecation, the location of permissions generally doesn't matter, and we've already moved them once from plone.app.controlpanel. The only scenario where this could be problematic is when someone explicitly includes permission.zcml. To avoid breaking anything, we should keep permission.zcml, not load it anymore, and include the new location as the only statement in the file. This way, we follow our deprecation guide and avoid any issues.