Closed mauritsvanrees closed 2 years ago
@mauritsvanrees thanks for creating this Pull Request and help improve Plone!
To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass.
Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:
@jenkins-plone-org please run jobs
With this simple comment all the jobs will be started automatically.
Happy hacking!
@jenkins-plone-org please run jobs
Note that this also means we pull in collective.folderishtypes
.
cc @plone/framework-team
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.
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: https://github.com/plone/plone.volto/issues/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.
Let's indeed wait with this. The checks are green, so that is a good start.
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?
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).
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
Since Volto will be the default interface for Plone 6, shouldn't the plone.volto
profile be installed by default?
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
?
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:
since Volto will be the default interface.
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 https://github.com/plone/Products.CMFPlone/pull/3344
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
I also think it is not the greatest idea to add plone.volto to Products.CMFPlone setup.py
Let's first find the common ground :-)
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).
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.
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?
- Now that
plone.volto
is a dependency ofPlone
, won't the variable HAS_VOLTO always beTrue
? 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.
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.
See also https://github.com/plone/Products.CMFPlone/issues/3371#issuecomment-983342380 and further comments.