BaseFieldDefinition::isStorageRequired wrongly falls back to ::isRequired

This patch removes the fallback to isRequired from within isStorageRequired.

In #2885809: The 'entity_type' and 'field_name' base fields on Comment are required the entity_type and field_name fields have been flagged as required, but not as storage required, which we should do not explicitly. However even if the fields have been flagged as required only, in the storage schema was checked if they are storage required before adding the NOT NULL constraint.

Adding a unit test to ensure that a required field is not automatically a storage required field.

I've just created a change record - https://www.drupal.org/node/3083233 .

But, changing it again now is just as problematic in the opposite direction, as evident by all the update functions and changes you have to do.

Could you please elaborate on what you see as problematic? Btw it is only a single update and not multiple update functions.

As discussed, the storage-level settings are not *meant* to be changed per-bundle.

Required is not a storage level setting and it never was. The documentation of setRequired or isRequired also was never updated that they will result into database NOT NULL constraints. Only the documentation of isStorageRequired explains that.

The test explicitly doesn't touch the storage required flag but changes only the required flag. I've also written this in the issue summary that both have different meanings.

Once again from the issue summary:

So now we have 2 methods for whether a field is required - isRequired and isStorageRequired, where the first is used for form inputs and validation purposes, while the second is used to flag the storage of the field in a shared table as required - for example by adding the NOT NULL constraint to the schema as described in the method's documentation.

I see this as a regression and the test demonstrates it. It was possible to make a base field required in a form and for validation purposes on one bundle and non-required on another bundle, which is a perfectly valid use case and I hope others see it like this as well.
Now because of mixing storage with entity and form validation this makes it impossible and therefore it is clearly a regression and not only this but suddenly NOT NULL constraints will be added around without them being explicitly requested (assumed one has implemented storage schema like documented in core).

Please note that the issue and the test deal only with the required and not the storage required flag. I have no intentions of changing a storage setting and core should neither have any intentions of putting a NOT NULL in the database constraints only because I've made a field required for the form and for validation purposes. It should be up to the developer to decide.

But the above approach should work fine to override it per bundle?

As I am not sure if I understand this correctly, I would ask you to describe exactly what you mean, so that there is no confusion :)

I still don't quite follow. The logic above this is an isset(), meaning you *can* set setRequiredStorage(FALSE) explicitly and then setRequired(TRUE)?

I am sorry, I've missed that sentence.

It is currently possible to (quoting you) "set setRequiredStorage(FALSE) explicitly and then setRequired(TRUE)". However one should explicitly turn off the storage required and this should happen before the entity schema is installed, because afterwards it will not have any effect and the NOT NULL constraint will be already there.

Quoting @jibran from #3003586-3: [PP-4] Use setStorageRequired() instead of overriding the storage schema to mark fields as NOT NULL in the database:

This is a huge DX improvement IMO but to remeber 'not null' => TRUE to setStorageRequired(TRUE) is not stright forward.

I fully agree with that. Imagine now that it gets much more complicated to remember that ::setRequired(TRUE) leads to ::isStorageRequired() returning TRUE without having called setStorageRequired(TRUE). And on top of that one has to think of that ::setRequired(TRUE) leads to a NOT NULL constraint on the storage level? This is a pure DX nightmare and makes things much harder to fix in the aftermath.

I'm kind of indifferent about this issue and the reason behind that is when you say the base field is required what does it really mean?

Does it mean FieldDefinition is required or FieldStorageDefinition is required?

It is really confusing now. However we have two methods for that - isRequired and isStorageRequired correspondingly.

BaseFieldDefinition and FieldConfig implement FieldDefinitionInterface on which isRequired() is documented:

Returns whether the field can be empty.

If a field is required, an entity needs to have at least a valid, non-empty item in that field's FieldItemList in order to pass validation.

This is what a required field / base field is and the main purpose is the validation.

Then we have RequiredFieldStorageDefinitionInterface which documents isStorageRequired() as

Returns whether the field storage is required.

If a field storage is required, NOT NULL constraints will be added automatically for the required properties of a field type.

RequiredFieldStorageDefinitionInterface is currently implemented only by BaseFieldDefinition.

I hope this clarifies the purpose of required and storage required and that as documented in the corresponding interfaces they have different purpose and therefore we should not implicitly fall back from the one to the other.

If a base field provider is making this decision that storage is not optional then it should set storage required explicitly instead of relying on field definition.

Exactly my point!

so that documentation is wrong at the moment in HEAD, which makes this more a documentation problem rather than a critical bug.

Well the documentation isn't completely wrong because depending on the output of \Drupal\Core\Field\RequiredFieldStorageDefinitionInterface::isStorageRequired() NOT NULL constraints are already being added in HEAD.

Even if we automatically do this everywhere the current issue remains a real problem and not only a documentation issue. I've explained in the issue summary why I consider this critical.

It is worth mentioning that regarding bundle fields only FieldConfig is aware of the required property and FieldStorageConfig does not know anything about it.

Only FieldConfigInterface extends the FieldDefinitionInterface where ::isRequired() is defined. The FieldStorageDefinitionInterface does not extend FieldDefinitionInterface.

This does not match BaseFieldDefinition where everything belongs together, but only on bundle fields we have a clear separation between what is a field definition setting and what is a field storage setting.

This issue is also a blocker for #3014760: [PP-2] Create an abstract composite field definition/storage definition and update BaseFieldDefinition to use it.
The proposal there is that the field storage definition is a property on the base field definition and as such it cannot call its parent from within ::isStorageRequired() and access ::isRequired().

Looks like I forgot to add the schema file...

I don't think #16 has been accounted for.

What exactly has not been addressed in my following comments #17 and #18?

I'm pretty uncomfortable with an change to entity storage that is going to necessitate contrib and custom writing update functions to ensure their expectations around data integrity are maintained. I'm going to ping @catch and @plach as core committers deeply involved in the entity system.

I don't think that anyone feels comfortable about this, but we should not forget that the change resolves the introduced regression and mitigates the adverse effects before we start providing NOT NULL constraints automatically for all entity types based on ::isStorageRequired().
We should think about developer reactions when they start receiving NOT NULL constraints in the storage just because they've flagged their fields required for entity and form validation. Or even worse, when they realize that it is impossible to save an entity form for a bundle where the requirement has been disabled.
I think that all this is much more important than the currently proposed change.

Additionally it also prevents a clear separation that is planned in #3014760: [PP-2] Create an abstract composite field definition/storage definition and update BaseFieldDefinition to use it.

The only explanation about the fallback from #2390495-11: Support marking field storage definitions as required is :

What if we make this a new optional interface, with a different method name (isSToragRequired()?). We can implement this in BaseFieldDefinition and actually call isRequired() by default, then someone who needs to do something weird can override it.

Which is not explaining the reason for the fallback in the first place.

In the very same comment you read the following as well, which has not been accounted for with the introduction of the fallback:

For configurable fields, it is now normal for the storage to not be required but the field definition/field config can be required.

What if a base field wants to provide a similar behavior? Make it optionally required per-bundle but not do that on the storage level?

This makes me think that it might have been just an oversight adding the fallback.

Is there a way we can deprecate and warn developers who depend on this behavior that they should update before the next release?

We could trigger a deprecation if the fallback returns TRUE, however how many releases from now should go on with this regression?

I would be fine with deprecating it in 8.8.x, but remove it in 8.9.0 where hopefully the automatic NOT NULL handling will be added.

If we deprecate it in 8.8.x then can we remove it in 8.9.x with a change record?

#2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field is ready (depends only one other issue which is RTBC) and hopefully can be committed to 8.9.x, however the current issue is a blocker for it as if someone decides to use the new schema they will get NOT NULL constraints unnoticed.

---

I could propose one more solution which might make it easier to commit earlier - next to the deprecation in 8.8.x, write an update that explicitly sets the storage_required flag to TRUE in the installed storage definitions if it has a NOT NULL constraint in the field schema for all tables it is contained in. This will ensure that the flag is kept on the installed storage definition in the cases someone relied on the fallback to ::isRequired(). During the update we could notify through a log message. We could even add a special detection for this in the EntityDefinitionUpdateManager::getChangeList(). This way only new installations will be affected, but not existing ones. Actually even without this existing ones will not be affected as they've already got the NOT NULL constraint in their databases if they relied on ::isStorageRequired().

---

I would therefore propose something like this.

Add the deprecation in 8.8.x if ::isStorageRequired() falls back to ::isRequired() and returns TRUE.
In 8.9.x remove the fallback and through an update set explicitly the storage_required flag to TRUE in the installed field storage definitions for required (::isRequired()) fields which had a NOT NULL constraints in their installed field schema and write a log message for each updated field. For this we should exclude the entity keys which by default get a NOT NULL constraint. Prior the release of 8.9.0 publish a change record about the change.
Additionally add a special detection for storage_required change in base fields in the EntityDefinitionUpdateManager::getChangeList(), which will be useful anyway.

Actually we could detect even better if a NOT NULL constraint is being added because of a fallback to ::isRequired() in ::isStorageRequired():

1. Retrieve all non entity key fields for which ::isStorageRequired() returns TRUE, but haven't being explicitly set as storage required.
2. Intersect those fields with the ones that have a NOT NULL constraint in at least one table in their installed field schema.
3. Retrieve a clone of the original field storage definitions for those fields and turn off the storage requirement - ->setStorageRequired(FALSE).
4. Retrieve the current field schema for the modified storage definition.
5. If a NOT NULL constraint is not added anymore then the fallback was used and we should explicitly update the installed storage definition with ->setStorageRequired(TRUE).

If a NOT NULL constraint is still added then this happens based on some other condition like the field name for example and there is nothing to do.

------
Edit:

I've proposed this as the most safe way, however given the fact that nothing really will break for existing site installations I don't think that such an update is justified.

New installations will simply not get the NOT NULL constraint in the database, but will still work