IField.required attribute is True but must not.

[jensens]

BUG/PROBLEM REPORT (OR OTHER COMMON ISSUE)

What I did:

We use IField https://github.com/zopefoundation/zope.schema/blob/master/src/zope/schema/interfaces.py#L192 with z3c.form to generate a form (plone.schemaeditor).

What I expect to happen:

The required of above schema interface attribute should render a field I can save checked or unchecked https://github.com/zopefoundation/zope.schema/blob/master/src/zope/schema/interfaces.py#L192

What actually happened:

The field can only be saved as selected, because it is required.

What version of Python and Zope/Addons I am using:

Plone with recent zc3.form

Analysis:

IField.required is a Bool field with required=True. This z3c.form does what it is told and render the field required.

A Bool fields default for required is True. It is inherited from Field https://github.com/zopefoundation/zope.schema/blob/master/src/zope/schema/_bootstrapfields.py#L222

Solution

Explicitly set IField.required Bool field definition to False like so:

    required = Bool(
        title=_("Required"),
        description=(_("Tells whether a field requires its value to exist.")),
        required=False,
        default=True)

[freddrake]

Is the problem that the widget doesn't provide a way to not set a value for the field?

IIRC, 'required' for a field is all about whether then attribute must be provided by the object described by the schema.

[jensens]

No, the widget is just the symptom showing up whats set at the field. The plone schema editor uses definition as-is. There is no (and also must be no) need to change this, because the editor has to strictly follow the definition. And the symptom, as said, is that it not work as expected.

[d-maurer]

Jens W. Klein wrote at 2021-1-29 08:53 -0800:

No, the widget is just the symptom showing up whats set the field. The plone schema editor uses definition as-is. There is no (and also must be no) need to change this, because the editor has to strictly follow the definition. And the symptom, as said, is that it work as expected.

The actual problem is that a `Bool` field it typically represented as a checkbox at the browser interface. A checkbox has only 2 states. Therefore, "unset" is interpreted as "no value provided" and as a consequence incompatible with "required". `zope.schema` has other use cases than browser interfaces. And at the Python level, "unset" and "no value provided" are distinguishable. There, `required` makes sense. Therefore, I strongly suggest to mark this change as a backward incompatible change.

[mauritsvanrees]

This has strange side effects when running the Plone 6 Jenkins tests. Jenkins gets confused and can't even show us how many tests failed in the overview. But let's pick one, that I can see locally too:

$ bin/test -s plone.rest
...
  File "/Users/maurits/shared-eggs/cp38/zope.configuration-4.4.0-py3.8.egg
/zope/configuration/config.py", line 869, in finish
    actions = self.handler(context, **args)
zope.configuration.xmlconfig.ZopeXMLConfigurationError:
File "/Users/maurits/community/plone-coredev/6.0/src/plone.rest/src/plone/rest/testing.zcml", line 163.2-167.6
    TypeError: cors_policy_directive() missing 1 required positional argument: 'allow_credentials'

This is with zope.schema 6.1.0. With 6.0.1 it succeeds.

allow_credentials is a Bool field in zcml.py, without any required keyword argument. It looks like it is intended to be required, because allow_credentials is a positional argument in the CORS policy directive in that same file.

In the tests this is used by testing.zcml, but this does not explicitly pass allow_credentials.

When I add required=False in the Bool field, it fails in the same way. When I add required=True the tests succeed, even though testing.zcml does not specify the allow_credentials field. I guess this may be because the field has a default (False). So I guess adding required=True is the right solution here for plone.rest.

If that is the only problem, then this change in zope.schema seems fine, but there are more failures, for example:

  File "/home/jenkins/.buildout/eggs/cp38/Products.GenericSetup-2.0.3-py3.8.egg/Products/GenericSetup/registry.py", line 438, in registerStep
    if already and already['version'] > version:
zope.configuration.config.ConfigurationExecutionError: File "/home/jenkins/workspace/plone-6.0-python-3.8/src/Products.CMFEditions/Products/CMFEditions/exportimport/configure.zcml", line 5.2-12.2
    <genericsetup:importStep
        name="cmfeditions_various"
        handler="Products.CMFEditions.setuphandlers.importVarious"
        title="Various CMFEditions Settings"
        description="CMFEditions specific settings">
      <depends name="toolset" />
      <depends name="typeinfo" />
    </genericsetup:importStep>
    TypeError: '>' not supported between instances of 'NoneType' and 'NoneType'

I don't see how to trigger this failure locally yet. Maybe the earlier plone.rest error simply trips up other tests.

[icemac]

As the fix seems to lead to other problems I am going to re-open this issue.

[jensens]

The meta level of definitions is hard, here and at plone.rest. I would consider it edge cases.

[mauritsvanrees]

Looks like the other failures were somehow fallout from the single problem in plone.rest. PR job for that was green.

[mauritsvanrees]

Jenkins stays green after merge. I close this.

[mgedmin]

Looks like this change broke my legacy app's test suite! zope.app.form's getWidgetsData() now omits Bool() fields from the returned data dictionary and I get KeyErrors in my code instead of False values.

Downgrading to zope.schema 6.0.1 fixes those tests.

I'm still trying to figure out what is happening. I think I'll cope, and there's no need to revert the change (but I reserve the right to change my mind ;).

[agroszer]

I'm not sure this was a good idea. On the business level I could have

class MakeSure:
    areYouSure = Bool(title='Are you sure?')

We can render the widget as a radio:

  Are you sure? *
  ( ) yes ( ) no

With 6.0.1 the user had to choose yes or no because Bool was required. With 6.1 the user can submit the form without picking one. It's my problem that I rely on field defaults, but honestly, who sets required True on a Bool?

Also of course z3c.form tests are failing like crazy because of this.

[mauritsvanrees]

collective.easyform also has problems. It no longer sends emails. See community report. Switching back to zope.schema 6.0.0 helps.

It is strange though: the PR for this issue seems to have more effects than simply changing if a field is required or not, even though that is exactly the only change that was done. In easyform, the checkbox for the standard mailer object is not checked anymore, and when I manually check it and save the form, the change is not persisted.
For Plone 5.2 I will go back to zope.schema 6.0.0.

[icemac]

Should this change get rolled back as it breaks more than it helps?

[icemac]

I just saw that in zopefoundation/z3c.form#104 the problems in z3c.form were already fixed.

[agroszer]

In my opinion it's a damn fundamental change, which can do much harm without being noticed. I fixed the z3c.form tests because we needed the MultiConverter fix.

The solution for our code was to add required to all Bool fields. But what's up with the rest of the world? Do we need to add required=True everywhere less those few places?

[mauritsvanrees]

But what's up with the rest of the world? Do we need to add required=True everywhere less those few places?

I think for the vast majority of Bool fields, required=False is their desired value. I have often added a Bool field, started the site, saw that the field was required, and had to go back to add required=False. So I agree with the intent of the change. It saves a bit of time by using the IMHO most often needed value.

But there are side effects, like the easyform issue I link to where there is a checkbox that you can select, but the value is not saved. I have not dived into why that is.

There can also be Bool fields like "I agree to the terms" that are intended to be required: you must check the box before you can successfully submit the form. When the field definition unknowingly relies on the old default (required=True), the form will now be broken.

Previously, all fields were required, unless you explicitly specified differently. It seems best not to change this suddenly.

@jensens What problem did this solve for you that could not have been solved by adding a required=False in a schema?

Quoting from your original comment:

What I expect to happen: The required of above schema interface attribute should render a field I can save checked or unchecked.
What actually happened: The field can only be saved as selected, because it is required.

My expectations are different. What actually happened originally is exactly what I expect.
So I would be in favour of rolling back this change.

[jensens]

Important here: The initial problem description is on the meta-model-level (and at least partly at meta-meta-level) of models (aka schemas in Zope). It was not a problem on the actual schema. Attention, this is confusing!

Note: it is because we define the meta-model (fields) the way as we define the model (schema) itself, using its own definitions. To break out, Python is our meta-meta level of the model

Change 1:
On the meta-level of Bool (so Bool as a schema) the inherited field "required" did not work, as stated at the top. Usually one does not matter, unless the schema is evaluated. This comes to the surface if one uses an schema-editor to create schemas and uses Bool as a schema to show a generated UI-form to edit schema. Exact this is what plone.schemaeditor does. The editor must work exactly on the raw meta-definitions of the fields. This was not possible. The change in https://github.com/zopefoundation/zope.schema/pull/105/files#diff-c59b46c6959e8de3ba33b2410a6907c642382ef616f1fc7891f8403126f9833fR407-R411 solves this. This change on the meta level is perfectly fine.

Change 2:
The direct schema level effect of the default value as discussed here in the last comments is this here.
https://github.com/zopefoundation/zope.schema/pull/105/files#diff-d23660616a31a398751772be7627843f88a4ed6e5b7a3bda8f5742511bc3fd73R613-R617
I agree, it is probably wrong and should be reverted.

ping @agitator

[mauritsvanrees]

That helps.
Obligatory xkcd meta reference.
Now let me see if I understand.

Say you define a schema with two fields: question is a TextLine, answer is a Bool.
IField.required is by default True. So by default all fields are required.
For the question field this is fine, but for the answer field not: you would be required to answer yes to every question. So you change its field definition to have required=False. All is well. This is how it was for years.

The problems start on the meta level.
IField.required is a Bool.
Bool is an instance of IBool.
IBool inherits from IField.
So it is a circle: Bool depends on itself. Bool is defined by a Bool.

I can't completely wrap my head around this, but I can imagine that this causes problems, along the lines of: you are required to have a required field, and it is required to have value True. Or slightly less confusingly said: you must have a field with name required and its only valid value is True.
At least, the danger is that you end up in a situation like that.
Your change was intended to fix this meta level, without meaning to change the 'normal' level.

I think it indeed makes sense to keep the first change, and revert the second change. Can you make a PR?

[jensens]

You're basically right, and yes, if it come to the meta (and meta meta) model levels things get a bit brain-twisting.

I'll create a PR.

[jensens]

merged