Add Plone Site: do a transaction commit before profiles

[mauritsvanrees]

When adding a Plone Site, do a transaction commit before applying extra profiles. This avoids errors in some cases when selecting add-ons in the advanced Plone Site add form, where installing them right after site creation in the add-ons control panel goes fine.

It should certainly fix some CSRF autoprotection errors raised by the first view after creating a Plone Site.

Fixes #2316.
And I think there was a similar issue raised recently, but I cannot find it.

Note for tests: since this does a transaction commit, any test code that calls addPloneSite should use a functional test layer, to avoid leakage into other tests. Case in point is an initial test failure in CMFPlone itself:

Error in test testPlonesiteWithoutUnicodeTitle
(Products.CMFPlone.tests.test_factory.TestFactoryPloneSite)
Traceback (most recent call last):
  File ".../unittest/case.py", line 329, in run
    testMethod()
  File ".../Products/CMFPlone/tests/test_factory.py", line 29,
 in testPlonesiteWithoutUnicodeTitle
    self.app, 'ploneFoo', title=TITLE, setup_content=False)
  File ".../Products/CMFPlone/factory.py", line 132, in addPloneSite
    context._setObject(site_id, PloneSite(site_id))
  File ".../OFS/ObjectManager.py", line 324, in _setObject
    v = self._checkId(id)
  File ".../OFS/ObjectManager.py", line 127, in checkValidId
    'The id "%s" is invalid - it is already in use.' % id)
BadRequest: The id "ploneFoo" is invalid - it is already in use.

I fixed that in the PR by adding a condition: when no extra profiles are selected, don't do a commit.
Only two test files in CMFPlone call addPloneSite directly, plus of course plone.app.testing in test layer setup, so this should not normally cause problems.

This PR is for master (5.2). I can port it to other versions once approved. I would say: only 5.1.

[jensens]

LGTM

[jensens]

+1 for backporting this to Plone 5.1

[zopyx]

Is it appropriate to commit here? addPloneSite() is supposed to be transactional. An error at some point of the setup procedure should not leave any traces...so what happens if an error occurs after the commit?

[mauritsvanrees]

Then you have a Plone Site without add-ons. That is probably acceptable here.

The only thing that happens after applying the extra profiles, is a redirect to the Plone Site url:
https://github.com/plone/Products.CMFPlone/blob/5.1.2/Products/CMFPlone/browser/admin.py#L265-L276

And... actually it should return an empty string after setting the redirect. Currently it also calls self.index(), and the redirect only really happens after the rendering is done. Rendering the index/template is unneeded, and might be a source of CSRF errors. Is that something you could try instead in a project where you see this problem?

[zopyx]

I don't think that it is acceptable...everything can happen during during the extensions profile after the commit. This will of course produce a naked Plone site but it would not produce what you expect. There is an unwritten contract that you should not commit yourself in Zope and Plone because this is handled by the ZPublisher. In that sense: addPloneSite() is no longer transactional. Also consider using addPloneSite() as part of a provisioning API (e.g. in plone.api). A remote API client can expect that a site is created according to its specs or not...but nothing in between...so I assume that committing here is a workaround at best...I would not consider this as a suitable solution.

[mauritsvanrees]

Ah, I found the other issue that I was looking for: #2228
Not exactly the same, but also a problem that only happens during initial site creation and not when using the add-ons control panel. Or at least that is the case for @ida. Maybe not for @zopyx who added that issue.
That issue is where I originally suggested a transaction commit, which apparently helped for Ida.

[mauritsvanrees]

I had another look and I see that the transaction commit doesn't really help. It only avoids printing several tracebacks, so that you are more likely to notice the original traceback that was always visible if you cared to scroll up far enough.

I will revert this PR.

[mauritsvanrees]

Directly reverted on master in 4e67724.