IField.required attribute is True but must not. about zope.schema HOT 19 CLOSED

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.

from zope.schema.

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.

from zope.schema.

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.

from zope.schema.

from zope.schema.

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.

from zope.schema.

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

from zope.schema.

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

from zope.schema.

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

from zope.schema.

Jenkins stays green after merge. I close this.

from zope.schema.

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 ;).

from zope.schema.

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.

from zope.schema.

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.

from zope.schema.

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

from zope.schema.

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

from zope.schema.

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?

from zope.schema.

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

from zope.schema.

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?

from zope.schema.

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.

from zope.schema.

merged

from zope.schema.