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

Depend on plone.volto in Plone 6 #20

Merged
merged 1 commit into from
Feb 24, 2022
Merged

Conversation

mauritsvanrees
Copy link
Sponsor Member

See also plone/Products.CMFPlone#3371 (comment) and further comments.

@mister-roboto

This comment has been minimized.

@mauritsvanrees
Copy link
Sponsor Member Author

@jenkins-plone-org please run jobs

@mauritsvanrees
Copy link
Sponsor Member Author

Note that this also means we pull in collective.folderishtypes.
cc @plone/framework-team

@ale-rt
Copy link
Member

ale-rt commented Dec 1, 2021

Having collective.folderishtypes in the play is really bad IMO.
I did not analyze fully the consequences of adding it into the play.
I do not see the point in pushing hard for this PR all of a sudden.

Can't we wait for the next alpha?

The @plone/framework-team already discussed the folderish types issue months ago, see https://community.plone.org/t/fwt-meeting-minutes-2021-09-21/14362.

I am quite against merging this PR.
I have to say that Plone 6 is still in its alpha phase.
For this reason I will not bang my head in to a wall if this PR is merged.
But please, @plone/volto-team promise me that before we start the beta cycle the dependency from collective.folderishtypes will be gone.

@sneridagh
Copy link
Member

sneridagh commented Dec 1, 2021

I also think that we should not merge until c.folderishtypes is gone and integrated into plone.volto. This is something we all agreed before Sorrento, and we stood on it during the sprint.

We went a long way, here is the current status:
plone/plone.volto#1

but unfortunately, it got stuck. So we need contributors and people that take care of it. The @plone/volto-team has also its limits, and cannot be everywhere. This is a Python-based task, so if anyone could take care of this, it would be great. I have the feeling that get rid of c.folderishtypes is an easy task, with BBB code and all. Just anybody had time to tackle it.

The "Volto" side of Plone 6 is not exclusively a task responsibility of the Volto Team, but of all the community. Maybe we can make that stand at some point, in community.plone.org? Just talking out loud here.

@mauritsvanrees
Copy link
Sponsor Member Author

Let's indeed wait with this. The checks are green, so that is a good start.

@mauritsvanrees mauritsvanrees added this to the Plone 6.0 milestone Dec 1, 2021
@ale-rt
Copy link
Member

ale-rt commented Dec 1, 2021

Thanks @sneridagh for your insight.
I checked plone.volto and I see it just this:

ale@avo:~/Code/plone/projects/coredev/6.0/src/plone.volto$ rg collective.folde
setup.py
60:        "collective.folderishtypes[dexterity]",

src/plone/volto/dependencies.zcml
21:  <include package="collective.folderishtypes" />

src/plone/volto/profiles/default/metadata.xml
5:    <dependency>profile-collective.folderishtypes.dx:default</dependency>

No mention of collective.folderishtype in plone.restapi.
What is the problem in removing collective.folderishtype?
Some migration step missing?

@sneridagh
Copy link
Member

sneridagh commented Dec 2, 2021

Talking by heart here, but we need to transfer the DX support for folderish types from c.folderishtypes to plone.volto. Then add a migration step for changing the base classes reference of the objects (no clue if we can avoid that with some BBB code in place).

@mauritsvanrees mauritsvanrees marked this pull request as ready for review February 23, 2022 19:42
@mauritsvanrees
Copy link
Sponsor Member Author

plone.volto is in the tested packages on Jenkins since I merged my coredev PR just now.
Biggest other blocker was the collective.folderishtypes dependency, but that is gone.
I guess the road is clear to merging this PR and have Plone pull in plone.volto. Is anything missing?

Meanwhile:

@jenkins-plone-org please run jobs

@mauritsvanrees mauritsvanrees merged commit 89f0596 into master Feb 24, 2022
@mauritsvanrees mauritsvanrees deleted the depend-plone-volto branch February 24, 2022 16:51
@wesleybl
Copy link
Member

Since Volto will be the default interface for Plone 6, shouldn't the plone.volto profile be installed by default?

@mauritsvanrees
Copy link
Sponsor Member Author

Since Volto will be the default interface for Plone 6, shouldn't the plone.volto profile be installed by default?

What do you mean with this? Do you want it in the dependencies of Products.CMFPlone?

@wesleybl
Copy link
Member

What do you mean with this? Do you want it in the dependencies of Products.CMFPlone?

Yes, it should be dependency of Products.CMFPlone. And your profile should be here:

https://github.com/plone/Products.CMFPlone/blob/master/Products/CMFPlone/profiles/dependencies/metadata.xml

since Volto will be the default interface.

@mauritsvanrees
Copy link
Sponsor Member Author

We could discuss adding plone.volto as dependency in the setup.py of Products.CMFPlone. All its installation requirements are already pulled in by other packages, so no new dependencies are added. This may make sense. I would like the opinion of the @plone/framework-team on that.

It does add yet another package that imports from Products.CMFPlone, giving rise to possible circular imports/dependency problems. CMFPlone does not import from plone.volto though, it only checks in one place if the package is available. So should be okay. cc @jensens

I would definitely not add it in the metadata.xml though. This would mean that if you want Classic Plone, you have a create a Plone Site and then immediately uninstall plone.volto.
I already solved this in a different way: the 'Create a new Plone Site' button creates a Volto site when the plone.volto package is available, and gives you the option to create a Classic site instead. See plone/Products.CMFPlone#3344

@jensens
Copy link
Sponsor Member

jensens commented Feb 28, 2022

Yes, it should be dependency of Products.CMFPlone. And your profile should be here:

No, veto!

First, and most important, this would introduce a whole bunch of most ugly dependency indirections, primary with its dependency on plone.restapi, breaking whatever (like at least pip install-ability),

Then, we definitely want to have a Plone Core which is independent and maintainable.

Also we have two flavors "Plone" (with volto) and maybe a future plone.classicui package (we may find a better name).

In fact the other way around is the way to go: A clean dependency graph. Less dependencies in Products.CMFPlone, like I can imagine having portlet-code part of classic ui, but not of Plone (and think further here).

So overall, packages like Plone and a future PloneClassicUI are the way to go in order to maintain a clean dependency graph. Products.CMFPlone is something rarely used directly, only by people knowing whats they are doing, like using Plone-as-a-Framework.

Anyway, also if anyone still consider this a good idea, please turn this into a PLIP. But I doubt it would have a chance (@plone/framework-team may decide differently)

cc @MrTango @agitator @tisto @ericof

@ale-rt
Copy link
Member

ale-rt commented Feb 28, 2022

I also think it is not the greatest idea to add plone.volto to Products.CMFPlone setup.py

@ericof
Copy link
Sponsor Member

ericof commented Feb 28, 2022

Let's first find the common ground :-)

  • I agree that adding plone.volto to Products,CMFPlone could lead to circular dependencies in the future
  • I also agree with the idea of moving toward a clear separation of frontend packages from the main distribution,

But, otoh, it is really strange that our default UI for Plone 6 will not be installed with the core of our package, which will probably lead to confusion.

I can see a future where we have either a pip install Plone or a pip install Plone[classic], and Products,CMFPlone is clean without other UI dependencies, but for now plone.volto needs to be pulled from somewhere in the default installation (and Plone package is the correct place, as it is now).

@sneridagh
Copy link
Member

For now, the main purpose of this PR was "Plone 6 is shipped with Volto integration in place (plone.volto)", which is already fulfilled, so we are good for now.

Agreed that we should not install it (plone.volto profile) until the integrator chooses to do so. And leave the door open for extend that choosing to other optional installs (on the classic side).

For now, the "chooser" side is also covered too, so we have the form on site creation (Maurits' button) and the scripts here and there (eg. docker images) that trigger the profile installation. However, IMHO we should improve the "chooser" at some point before final release, and also include it in our scaffolding tools (if any). At least study, design and improve the experience on that side, and above of all, make it friendly and approachable for future integrators, newcomers and newbies alike.

@wesleybl
Copy link
Member

wesleybl commented Mar 3, 2022

About plone.volto not being a dependency of Products.CMFPlone, I understand your arguments, and I'm fine with that.

What I would find strange is having to install an extra package to use the default interface. But the Maurits button does this automatically. So I'm fine with that too.

Now some doubts:

  • Now that plone.volto is a dependency of Plone, won't the variable HAS_VOLTO always be True? Can't we remove it?

  • Although the Volto site is created by default via the web interface, the tests are run without the plone.volto profile being installed. Wouldn't it be nice for tests to be run with it installed, since Volto is the default interface?

@mauritsvanrees
Copy link
Sponsor Member Author

  • Now that plone.volto is a dependency of Plone, won't the variable HAS_VOLTO always be True? Can't we remove it?

No, because this HAS_VOLTO is defined in Products.CMFPlone, which does not depend on Plone (it is the other way around), so plone.volto is not always available.

  • Although the Volto site is created by default via the web interface, the tests are run without the plone.volto profile being installed. Wouldn't it be nice for tests to be run with it installed, since Volto is the default interface?

Interesting point. I fear that lots of tests would fail. Or they would not make sense anymore. Take for example a test that creates a page, adds some rich text in the text field (plone.app.textfield) and views the site in a test browser. If this test still works, you would get the html of the classic UI. But maybe the test setup fails because the field is not there anymore: only blocks are left. And you would really need to see the Volto UI, but the Python test setup will not start a node server.

Well, for starters it would be good to define a test layer in plone.app.testing that creates a site with plone.volto if available. Then packages can start creating explicit tests for this.

@wesleybl
Copy link
Member

wesleybl commented Mar 3, 2022

Well, for starters it would be good to define a test layer in plone.app.testing that creates a site with plone.volto if available. Then packages can start creating explicit tests for this.

Maybe the default layer should create a site Volto and have a additional layer to create a classic site.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants