Running upgrade step results in Plone's default ``front-page`` being recreated

thet commented 6 years ago

What I did:

Run my site's integration profile with the option to include all dependent profiles. Since I have a custom workflow, which does not have the publish transition (instead a make-public one), the following error appeared:

2017-12-01 04:35:11 ERROR Zope.SiteErrorLog 1512099311.640.492587541282 http://localhost:8000/iaem/iaemportal/portal_setup/manage_importAllSteps
Traceback (innermost last):
  Module ZPublisher.Publish, line 138, in publish
  Module ZPublisher.mapply, line 77, in mapply
  Module ZPublisher.Publish, line 48, in call_object
  Module Products.GenericSetup.tool, line 579, in manage_importAllSteps
  Module Products.GenericSetup.tool, line 388, in runAllImportStepsFromProfile
   - __traceback_info__: profile-iaem.site:default
  Module Products.GenericSetup.tool, line 1433, in _runImportStepsFromContext
  Module Products.GenericSetup.tool, line 1245, in _doRunImportStep
   - __traceback_info__: plone.app.contenttypes--plone-content
  Module plone.app.contenttypes.setuphandlers, line 331, in step_import_content
  Module plone.app.contenttypes.setuphandlers, line 199, in create_frontpage
  Module plone.app.contenttypes.setuphandlers, line 49, in _publish
  Module Products.CMFCore.WorkflowTool, line 234, in doActionFor
WorkflowException: No workflow provides the '${action_id}' action.

OK, so i changed the default workflow temporarily to simple_publication_workflow and re-run the profile.

The following below happened.

What I expect to happen:

My profile to be applied successfully without any side effects.

What actually happened:

My profile was applied, the workflow was reset to mine, but:

I have set a default page (id = index) - the default Plone landing page (id = front-page) was deleted.
The default page was set private.
instead the previously deleted front-page was recreated.
@@hero wasn't found and resulted in a Diazo error:

2017-12-01 04:36:19 ERROR plone.subrequest Error handling subrequest to /@@hero
Traceback (most recent call last):
  File "/home/iaem/iaem-buildout/eggs/plone.subrequest-1.8.4-py2.7.egg/plone/subrequest/__init__.py", line 145, in subrequest
    traversed = request.traverse(path)
  File "/home/iaem/iaem-buildout/eggs/Zope2-2.13.26-py2.7.egg/ZPublisher/BaseRequest.py", line 525, in traverse
    return response.notFoundError(URL)
  File "/home/iaem/iaem-buildout/eggs/Zope2-2.13.26-py2.7.egg/ZPublisher/HTTPResponse.py", line 718, in notFoundError
    "<p><b>Resource:</b> %s</p>" % escape(entry))
NotFound:   <h2>Site Error</h2>
  <p>An error was encountered while publishing this resource.
  </p>
  <p><strong>Resource not found</strong></p>

  Sorry, the requested resource does not exist.<p>Check the URL and try again.</p><p><b>Resource:</b> http://localhost:8000/iaem/iaemportal/@@hero</p>
  <hr noshade="noshade"/>

  <p>Troubleshooting Suggestions</p>

  <ul>
  <li>The URL may be incorrect.</li>
  <li>The parameters passed to this resource may be incorrect.</li>
  <li>A resource that this resource relies on may be
      encountering an error.</li>
  </ul>

  <p>For more detailed information about the error, please
  refer to the error log.
  </p>

  <p>If the error persists please contact the site maintainer.
  Thank you for your patience.
  </p>
2017-12-01 04:36:19 ERROR plone.app.theming Couldn't resolve /@@hero
I/O warning : failed to load external entity "/@@hero"

What version of Plone/ Addons I am using:

5.1rc2-pending

Comments

Without any further investigation I suspect a plone.app.contenttypes upgrade step responsible for this.

@pbauer maybe you know more on that?

pbauer commented 6 years ago

I think I figured it out.

I tried to reproduce the issue in 5.1rc1 and also in 5.1rc2-pending I failed. But when I upgraded a site from 5.1rc1 to 5.1rc2 it actually happened.

The reason is the change to use a post_handler instead of a import_step in https://github.com/plone/plone.app.contenttypes/commit/322ae669eafc6036e2b0dd32409488a7ff077d7b and how GenericSetup handles such steps.

In 5.1rc1 the key plone.app.contenttypes--plone-content is registered in portal_setup._import_registry._registered because it is still a import_step. In 5.1rc2 it is no longer a import_step but a post_handler but the registration is still there. So the method is run. In 5.1rc1 it does nothing because the context in which it is run is the profile of your package and the test if the marker-file is in the profile-dir is failing, aborting the method before doing anything. In 5.1rc2 this check is no longer there since we rely on GenericSetup to run post_handlers only when applying exacly the profile in question.

The solution is to add a upgrade-step that removes any registration of plone.app.contenttypes-- plone-content when migrating to 5.1rc2.

Maybe @mauritsvanrees should be aware of that issue.

agitator commented 6 years ago

I'm also experiencing this problem, kudos for finding that out!

pbauer commented 6 years ago

@thet can you check if your issue is gone with https://github.com/plone/plone.app.upgrade/pull/144?

mauritsvanrees commented 6 years ago

Heh, nice one. Registering import steps in import_steps.xml is really old and should not be done, though I guess there might be reasons. If these had been registered in zcml, this would not have happened.

Your fix looks good. I will comment there.

Manual workaround would be to go to the Manage tab of portal_setup (portal_setup/manage_stepRegistry) and delete the steps there.

pbauer commented 6 years ago

@mauritsvanrees I can confirm that we need to run this before every other step to fix this issue. As you suspected in https://github.com/plone/plone.app.upgrade/pull/144#pullrequestreview-82301681

Any ideas on how to implement this in a sane way? I experimented with a list of steps that need to run before all others and adding them to the top of the list of steps. But I ran into several issues that made me hate my code:

the id of a upgrade-step is generated from some additional info so I had to store title, source, target and sortkey (breaks when a title is changed)
the step needs to run through several methods to be in the right format which makes it hard to read (_upgrade_registry.getUpgradeStep, _extractStepInfo and _massageUpgradeInfo)
after running each step the profile version is set to that steps destination-version. If the next step executes code that for some reason checks the profile version it might break.

Should we instead try not to be too clever and run the a method pre_steps directly that runs the handlers directly?

pbauer commented 6 years ago

A workaround would be to simply rename the steps that I migrated in https://github.com/plone/plone.app.contenttypes/commit/322ae669eafc6036e2b0dd32409488a7ff077d7b to something else. This way the old steps will not be found and can safely be removed in cleanup_import_steps.

I implemented that in https://github.com/plone/plone.app.contenttypes/pull/445

mauritsvanrees commented 6 years ago

It could help if the MigrationTool would have a list of functions to call before running any steps. Something like this:

RUN_ME = [
    fix_stuff,
]

class MigrationTool:
    def run(...)
        for method in RUN_ME:
            method(self)  # or portal or portal_setup
        ... continue with normal migration run

Maybe some functions are already there by default. Maybe other packages, like plone.app.contenttypes here, should be expected to append a function here. plone.app.upgrade may add a few. Probably the RUN_ME list should not be added to directly, but via a small function, so that it can more easily be refactory, for example if a sort order is needed:

_RUN_ME = []

def add_pre_step(method, position=500):
    _RUN_ME.append((position, method))
    _RUN_ME = sorted(_RUN_ME)

mauritsvanrees commented 6 years ago

I created an issue for that: https://github.com/plone/Products.CMFPlone/issues/2246

plone / Products.CMFPlone