PLIP 3395 plone.base: Untangles existing circular dependencies (imports/zcml) in Plone.

[jensens]

PLIP (Plone Improvement Proposal)

Dear @plone/framework-team - I am happy to submit this PLIP

Responsible Persons

Proposer: @jensens

Seconder: @mauritsvanrees

Abstract

This PLIP is a major step toward the cleanup of our dependency mess in Plone core. It untangles the (implicit) dependencies on import/ ZCML level and ensures a clean dependency graph.

Motivation

After discussion around PLIP #3355, the result was to start with a new plone.base package to resolve existing circular dependencies.
Current circular dependencies between Product.CMFPlone and several plone.app.* and some Products.* packages are inconvenient and wrong. My heart is bleeding with this architecture. In #3355

Assumptions

Product.CMFPlone is consumer of (IOW it imports) most plone.* and some Products.* packages. If it provides functionality, this is done by adapters and utilities only.

Most circular dependencies are between other core and Product.CMFPlone modules/packages

• interfaces.*
• permissions
• utils
• PloneMessageFactory in NS-root.
• PloneBatch
• defaultpage
• i18nl10n

That said, some packages have special needs, here an analysis of all packages:

• plone.app.contentlisting: utils, interfaces and Batch is used. Latter could live in plone.base or moved to plone.batching. Plone base overhaul plone.app.contentlisting#43
• plone.app.contentmenu: utils, interfaces Plone base overhault plone.app.contentmenu#34
• plone.app.contentrules:
  • utils, interfaces Plone base overhaul plone.app.contentrules#71
  • breadcrumbs
  • strange five_template usage
• plone.app.contenttypes:
  • utils, interfaces, also Batch
  • and new: CMFPlone.browser.syndication.adapters.CollectionFeed.
• plone.app.dexterity utils, interfaces, Batch Plone base overhaul plone.app.dexterity#348
• plone.app.discussion utils, interfaces, Batch, DISCUSSION_ANNOTATION_KEY (could be moved to pa.discussion) Plone base overhaul plone.app.discussion#195
• plone.app.layout Base overhaul plone.app.layout#302
  • utils, interfaces,
  • also CMFPlone.browser.syndication.adapters.*
  • and CMFPlone.defaultpage.check_default_page_via_view
• plone.app.multilingual - utils, interfaces and CMFPlone.defaultpage.is_default_page. This one is an optional addon anyway. Should be moved as a dependency of Plone package, there are only few traces in CMFPlone anyway. That way it would be a consumer of Products.CMFPlone so it could depend directly on Products.CMFPlone (which is not necessary).
• plone.app.registry - utils, interfaces. Plone.base overhaul plone.app.registry#63
• plone.app.theming
  • utils, interfaces
  • and CMFPlone.resources.
• plone.app.users
  • utils, interfaces, Batch
  • and CMFPlone.controlpanel.events.ConfigurationChangedEvent,
  • CMFPlone.RegistrationTool.EmailAddressInvalid
• plone.app.workflow: interfaces, utils. Plone base overhaul plone.app.workflow#32
• plone.app.event: interfaces.utils and CMFPlone.i18nl10n. Plone.base overhaul plone.app.event#356
• plone.app.linkintegrity: interfaces.
• plone.app.querystring: utils and interfaces
• plone.app.redirector: single interface import
• plone.app.relationfield: single interface in ZCML
• plone.app.textfield: one util, one interface usage
• plone.app.versioningbehavior: one usage of utils
• plone.app.widgets: one use of utils
• plone.app.z3cform: interfaces and utils

Packages which are OK as dependencies of CMFPlone

• plone.app.i18n
• plone.app.intid
• plone.app.lockingbehavior
• plone.app.uuid

Packages which are OK as pure consumers of CMFPlone:

• plone.api
• plone.app.upgrade
• plone.app.caching
• plone.app.iterate

Packages with circular dependencies not primary addressed by this PLIP. We try to fix them along anyway if possible without major efforts.

• plone.app.vocabularies - it is a nice idea to bundle all vocabularies here, but in fact they should be placed where semantically belonging to, except the generic ones in there. I.e. if plone.app.querystring has data to be presented as vocabulary, well, put it there. This can probably be untangled with some effort. Same for Image sizes CMFPlone.utils.getAllowedSizes dependency.
• plone.app.content
  • given utils and interfaces are in plone.base
  • then also Products.CMFPlone.DublinCore is needed here. Question is how much we need from DublinCore and if it can go in to plone.dexterity? Need research.
• plone.app.contentrules: Furthermore CMFPlone.browser.navigation.PhysicalNavigationBreadcrumbs and Products.CMFPlone.Portal.PloneSite (in zcml) is used.
• plone.app.viewletmanager: CMFPlone.exportimport.tests.base.BodyAdapterTestCase

Proposal & Implementation

A new package plone.base will be created. It contains at least:

• interfaces.*
• permissions
• utils
• PloneMessageFactory in NS-root.
• batch
• i18nl10n

For each moved package a deprecation will be created according to the best practices in our Plone Deprecation Guide.

Already deprecated functionality from Plone 5 wont be moved but removed.

Imports will be fixed in Products.CMFPlone.

In a second step all core packages will be fixed to use the new plone.base.

If there are tests for the moved functionality and it is feasible to move those w/o major efforts, it will be moved to, otherwise it stays in CMFPlone.

Deliverables

The PLIP will be merged in two steps:

1. new plone.base and Products.CMFPlone - this should happen in alpha phase!
   PLIP config at https://github.com/plone/buildout.coredev/blob/6.0/plips/plip-3395-plone.base-part-1.cfg

2. adopt package by package to fix imports and remove deprecation warnings

Risks

• yet another package instead of package reduction :(
• in registry we have entries pointing to the old location. This is not a problem as it seems, but not that beautiful. Migration is possible, but overrides in add-ons may suffer from this.
• lots of deprecation messages in add-ons.

Participants

• @jensens
• add yourself

[jensens]

I implemented already the major part of it.

• Plip 3395 plone base #3396
• https://github.com/plone/plone.base

[gforcada]

Cool to see this moving forward!! 🎉 💯

One question from a CI point of view: the goal is of course to get rid of the circular dependencies, but do we have a way to ensure we are not adding them back yet? 😅

Should we add another check on our CI, that tests, only on an approved list of the sorts (to avoid too much noise), that packages do not get themselves tangled in a circular dependency? 🙂

[jensens]

@gforcada I think this is a good idea. I am not familiar with the tools to analyse on import level (and as bonus ZCML!) if we run into circular dependencies.

[gforcada]

ZCML is indeed a bit more complex... for import level, if we use z3c.dependencychecker to get setup.py dependencies in shape, it could be noticed(?) with a simple pip install? 🤔 but there has to be some tool around... we will not be the first ones trying to get out of a circular dependency mess

[jensens]

Well, checking setup.py is not enough. We already cleaned those up to not have circulars, but imports are going back anyway. So we would need a checker on Python level, parsing the packages code (AST?) and looking at all imports.

[gforcada]

that's exactly what z3c.dependencychecker does, we use it internally (no code changes) on our CI pipeline to avoid merging code that imports/uses code that is not declared on setup.py ✨

[mauritsvanrees]

I have added myself as seconder of this PLIP.

Status: Jens mostly finished this in January already. In the Plone 6 coordination meeting today, he said he wants to wait until all the ES6 stuff is merged, and then finish it.

Note that there is not yet an initial approval of the idea by the @plone/framework-team.

[ale-rt]

This PLIP has been approved in some follow up emails after the FWT meeting the happened the 10th of March.

[mauritsvanrees]

This would need an upgrade step in plone.app.upgrade. All keys in the registry that start with Products.CMFPlone.interfaces should be changed to plone.base.interfaces. Right?

See v52.final.change_interface_on_lang_registry_records where we do the same for the update from CMFPlone.interfaces to plone.i18n.interfaces.

[mauritsvanrees]

Also, we should be careful that we do not clean this up and then reintroduce old interfaces later on, for example when running a plone.staticresource upgrade step. But if we forget something, we can always call this cleanup step a second time during beta.

[jensens]

All keys in the registry that start with Products.CMFPlone.interfaces should be changed to plone.base.interfaces.

This would be necessary, but it is not necessary for the next alpha.

[jensens]

Short review by @ale-rt here: #3396

[jensens]

part 1 merged

[mauritsvanrees]

@jensens Can you make a PyPI release of plone.base please? I would suggest version 1.0.0a1.
Please give the plone user the Owner role.

Without a release, currently the plone.restapi GHA fails because CMFPlone is checked out, but plone.base is nowhere to be found. I am fixing this in a different way in plone/plone.restapi#1358

[jensens]

@maurits wouldn't it be better to fix the GHA over there to use buildout.coredev? I anyway wondered why such a non standard layout is used there.
Anyway, I can do a release if needed tomorrow.

[ale-rt]

@maurits wouldn't it be better to fix the GHA over there to use buildout.coredev? I anyway wondered why such a non standard layout is used there.

See:

• Checkout Products.CMFPlone to have a green GHA build again plone.restapi#1255
• Fix GHA test action to just run tests on Plone 5.2. Plone 6 CI is bes… plone.restapi#1259

That test setup will be a recurring pain

[jensens]

This is done, improvements are still possible. Left are deprecation warnings. I tend to close this one and do so.