plone / plone.registry

provides debconf-like (or about:config-like) settings registries for Zope applications
https://pypi.org/project/plone.registry
1 stars 3 forks source link

Custom validator will prevent records from being reimported #21

Open frapell opened 5 years ago

frapell commented 5 years ago

Steps to reproduce:

  1. Create a control panel following instructions from https://pypi.org/project/plone.app.registry/#creating-a-custom-control-panel
  2. Add a validator for a field as explained in https://docs.plone.org/develop/addons/schema-driven-forms/customising-form-behaviour/validation.html#field-widget-validators
  3. Add initial values for that same field in registry.xml
  4. Install the product

You will see a traceback like this:

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.CMFPlone.controlpanel.browser.quickinstaller, line 213, in __call__
  Module <string>, line 3, in installProducts
  Module AccessControl.requestmethod, line 70, in _curried
  Module Products.CMFQuickInstallerTool.QuickInstallerTool, line 686, in installProducts
  Module Products.CMFQuickInstallerTool.QuickInstallerTool, line 602, in installProduct
  Module Products.GenericSetup.tool, line 388, in runAllImportStepsFromProfile
  Module Products.GenericSetup.tool, line 1433, in _runImportStepsFromContext
  Module Products.GenericSetup.tool, line 1245, in _doRunImportStep
  Module plone.app.registry.exportimport.handler, line 70, in importRegistry
  Module plone.app.registry.exportimport.handler, line 116, in importDocument
  Module plone.app.registry.exportimport.handler, line 324, in importRecord
  Module plone.registry.record, line 34, in __init__
ValueError: Field is not persistent

If the product was installed and the validator is added afterwards, when you try to reimport the registry.xml the traceback looks like this:

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 559, in manage_importSelectedSteps
  Module Products.GenericSetup.tool, line 358, in runImportStepFromProfile
  Module Products.GenericSetup.tool, line 1245, in _doRunImportStep
  Module plone.app.registry.exportimport.handler, line 70, in importRegistry
  Module plone.app.registry.exportimport.handler, line 116, in importDocument
  Module plone.app.registry.exportimport.handler, line 282, in importRecord
  Module plone.registry.record, line 60, in _set_field
  Module plone.registry.registry, line 283, in _setField
ValueError: The record's field must be an IPersistentField.

This is what happens in https://github.com/collective/collective.z3cform.datagridfield/issues/14

As I explain in my comment (https://github.com/collective/collective.z3cform.datagridfield/issues/14#issuecomment-454530555), I found that when the persistent field is created, the __provides__ attribute would be overridden. This only happens when the field has a custom validator defined.

In order to fix it, I have added the __provides__ to the list of ignored attributes by adding to my __init.py__:

from plone.registry.field import DisallowedProperty
DisallowedProperty('__provides__')

I found that the vocabulary attribute is already ignored in https://github.com/plone/plone.registry/blob/4d03736d8cf41ffed2323ea6b6d40d2c4b6f7364/plone/registry/field.py#L280

should I add the __provides__ here as well as a solution?

frapell commented 5 years ago

@mauritsvanrees What do you think?

mauritsvanrees commented 5 years ago

The way you put it, such a change would seem to make sense.

But wouldn't this raise a ValueError here? Ah, probably not, because the property would no longer be set, but be filtered out. Seems okay then.

I wonder if we should disallow anything starting with an underscore. That would probably mean changing https://github.com/plone/plone.registry/blob/4d03736d8cf41ffed2323ea6b6d40d2c4b6f7364/plone/registry/fieldfactory.py#L43-L45 into something like this:

  context_dict = dict(
        [(k, v) for k, v in context.__dict__.items() if k not in ignored
        and not k.startswith('_')]
    )

I don't know, I don't remember having touched any code in plone.registry, or having run into problems with it.