search

plone / Products.CMFPlone

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

Circular dependency between p.a.content and plone.app.dexterity #3858

Closed gforcada closed 9 months ago

gforcada commented 10 months ago

That's an interesting one 😄

plone.app.dexterity

It uses the following from plone.app.content:

plone/app/dexterity/behaviors/configure.zcml:67:      provides="plone.app.content.interfaces.INameFromTitle"
plone/app/dexterity/behaviors/filename.py:1:from plone.app.content.interfaces import INameFromTitle

plone/app/dexterity/tests/namefromtitle.txt:15:plone.app.content (normally this would be done via Generic Setup)::
plone/app/dexterity/tests/namefromtitle.txt:17:  >>> fti.behaviors = ('plone.app.content.interfaces.INameFromTitle',
plone/app/dexterity/tests/test_constrains.py:1:from plone.app.content.browser.constraintypes import IConstrainForm
plone/app/dexterity/tests/test_permissions.py:1:from plone.app.content.browser.vocabulary import VocabularyView

i.e. basically a single interface (INameFromTitle) and on tests, where is fair game IMHO.

plone.app.content

It uses the following from plone.app.dexterity:

plone/app/content/browser/contents/properties.py:4:from plone.app.dexterity.behaviors.metadata import ICategorization
plone/app/content/browser/file.py:3:from plone.app.dexterity.interfaces import IDXFileFactory

plone/app/content/tests/test_contents.py:39:            "plone.app.dexterity.behaviors.metadata.IBasic",
plone/app/content/tests/test_contents.py:82:            "plone.app.dexterity.behaviors.metadata.IBasic",
plone/app/content/tests/test_contents.py:165:            "plone.app.dexterity.behaviors.metadata.IBasic",
plone/app/content/tests/test_contents.py:266:            "plone.app.dexterity.behaviors.metadata.IBasic",
plone/app/content/tests/test_contents.py:349:        type1_fti.behaviors = ("plone.app.dexterity.behaviors.metadata.IBasic",)
plone/app/content/tests/test_contents.py:358:        type2_fti.behaviors = ("plone.app.dexterity.behaviors.metadata.IBasic",)
plone/app/content/tests/test_widgets.py:683:        # the next calls plone.app.dexterity.factories and does a
plone/app/content/tests/test_widgets.py:700:        # the next calls plone.app.dexterity.factories and does a

So, two imports mainly: ICategorization and IDXFileFactory, and again, some tests...

Proposal

plone.app.content is meant to be on top of plone.app.dexterity, thus a solution can be:

davisagli commented 10 months ago

In my opinion plone.app.content belongs above plone.app.dexterity. plone.app.dexterity is the core content type framework, while plone.app.content is mostly the Classic UI folder contents view. This can probably be resolved by moving INameFromTitle to plone.base, and any other frontend-agnostic functionality from plone.app.content to either plone.app.dexterity or Products.CMFPlone.

gforcada commented 10 months ago

Oh, thanks, yes, that makes more sense indeed.

Other than INameFromTitle the other imports on plone.app.dexterity are on tests that actually could be moved directly on plone.app.content itself, as they are testing the functionality of plone.app.content 🤔

https://github.com/plone/plone.app.dexterity/blob/master/plone/app/dexterity/tests/test_permissions.py

https://github.com/plone/plone.app.dexterity/blob/master/plone/app/dexterity/tests/test_constrains.py

gforcada commented 10 months ago

@davisagli @jensens @mauritsvanrees how does the Proposal section above in the first comment look like? On my head it makes all sense, and it can be done in small PRs that should not break Jenkins at any point 🍀

davisagli commented 10 months ago

@gforcada Makes sense to me, that's what I was thinking too

mauritsvanrees commented 10 months ago

Sounds good to me.