Support marking field storage definitions as required

Agreed.

Reparenting this issue since we moved the scope of needing this from the original parent to the new one.

Related : #2411323: FieldStorageConfig::isRequired should not exist

Since #2232477: Fatal when adding new fields with NOT NULL constraints in a base table that contains existing entities, not null constraints are added to all required properties of a field type. Is there anything else we need to do here?

Answering to myself in #5, the isRequired() method is actually needed for #2346019: Handle initial values when creating a new field storage definition, so here's a patch for that.

Note that the documentation that I added to \Drupal\Core\Field\FieldStorageDefinitionInterface::isRequired() is lying a bit, NOT NULL constraints will only be added in #2346019: Handle initial values when creating a new field storage definition.

#8 Looks good to me

+1

We have isRequired() now on FieldDefinitionInterface and on FieldStorageDefinitionInterface. Are we sure that we won't get a conflict on that, for example for base fields?

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?

Would it be better to have a separate method?

Also, the fact that we have this method already on base fields makes me wonder if this isn't a hidden BC break. We have somewhat relaxed our rules that we can add new methods e.g. to entity (type) interfaces as we have base classes there which can provide sane defaults that we can expect people to implement. That's currently not the case for FieldStorageDefinitionInterface as we do not have a base class that could provide sane defaults, beside BaseFieldDefinition (#2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info() as well as @todo on \Drupal\entity_test\FieldStorageDefinition).

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.

And we can also simply not implement it with FieldStorageConfig, which I think would technically also be more correct (It's not that we know that it is not required, we actually don't know if it is or not)

@Berdir is right, the clash in BaseFieldDefinition with the existing 'required' property is unfortunate and I like the suggestion from #11, let's do that :)

Oops, that's an IDE autocompletion fail :)

+++ b/core/lib/Drupal/Core/Field/RequiredFieldStorageDefinitionInterface.php
@@ -0,0 +1,32 @@
+
+  /**
+   * Sets whether the field storage is required.
+   *
+   * @param bool $required
+   *   Whether the field storage is required.
+   *
+   * @return static
+   *   The object itself for chaining.
+   */
+  public function setStorageRequired($required);

FieldStorageDefinitionInterface on purpose only has get*, has*, is*. It doesn't specify how those are set as you are usually not supposed to change those values when working against the interface, e.g. changing a base field definition isn't allowed outside of the places where you may define/alter it.

So I think we should only have the is method on the interface?

That's perfectly true and reasonable :)

Late night coding...

The testbot has finally decided to grant us a good run here :)

Yeah, that makes sense to me.

This doesn't do anything yet on its own, but #2346019: Handle initial values when creating a new field storage definition will depend on it.

I guess setting it to RTBC will start to fail again on testbot... :(

We need a CR for this addition.

I've added a change record for this https://www.drupal.org/node/2841350

Removing Needs change record tag

Committed cc30d65 and pushed to 8.3.x. Thanks!