hook_entity_base_field_info_alter() and hook_entity_bundle_field_info_alter() are documented to get a parameter that doesn't implement an interface with setter methods

I've closed #2225961: Introduce "controlled" setters on FieldDefinitionInterface to avoid special-casing FieldDefinition and #2143297: setters on FieldDefinitionInterface as a duplicate of that this, they might have information that could be relevant.

Discussed with @effulgentsia and @alexpott. This is definitely buggy (@effulgentsia pointed out that we have an invalid code example for hook_entity_base_field_info_alter() in entity.api.php even), but the actual functional failures are likely edgecases that have workarounds, so this shouldn't block the release of Drupal 8.0.0.

This is still a major issue for all the reasons in #4. But it needs a beta eval and some architectural insight.

Also tagging as related: #2225961: Introduce "controlled" setters on FieldDefinitionInterface to avoid special-casing FieldDefinition

http://drupal.stackexchange.com/questions/156999/creating-account-type-w...

We still have in-code @todos pointing to #2225961: Introduce "controlled" setters on FieldDefinitionInterface to avoid special-casing FieldDefinition, which is marked as a dupe of this. Just FYI.

Can't you copy an existing one and recreate one?

For that we would need something like a toArray that you can pass to a factory to facilitate an easier copying.

Discussed this today with @xjm, @plach, @amateescu and @Berdir and we agreed to downgrade this. There is no functional bug that has been pointed out here, nothing that is actually known not to work, neither in core or in contrib. So we should definitely fix this in some way, but the objects that you get passed do actually have various methods that you need, it's just not typehinted.

We also explicitly agreed that this would be re-raised to major if someone can point out a case where something is being passed that actually does not have the proper methods that you need.

Oh, I think this no longer needs a beta evaluation, by the way ;-)

These kind of issues are quite complex to unpick, I think it would almost be worth dealing with them in two separate issues. Trying to tackle base fields first.

Reading over the implementation of EntityFieldManager::buildBaseFieldDefinitions, things happen in roughly this order:

• $entity_class::baseFieldDefinitions is invoked to get the initial list of fields (this hints FieldDefinitionInterface).
• Some additional fields are added depending on the circumstances.
• entity_base_field_info is invoked (also hints FieldDefinitionInterface).
• entity_base_field_info_alter is invoked (also hints FieldDefinitionInterface).

Here is my analysis of what the current state of this issue is:

• Right now core seems to only require FieldDefinitionInterface, it doesn't assume BaseFieldDefinition outside of a bug in some documentation.
• In reality, developers expect a BaseFieldDefinition because that's what everything uses at the moment.
• There would be nothing stopping me from creating an implementation of FieldDefinitionInterface that had no setters, except that alter hook implementations would be left scratching their head.

I don't believe any of the alter hooks are any less useful hinting FieldDefinitionInterface interface. You are still completely able to replace the implementation of a field using setters in an API way like so:

  $fields['altered_field'] = BaseFieldDefinition::create($fields['altered_field']->getType())->...

Perhaps the issue should be to fix all documentation to make it clear an implementation of FieldDefinitionInterface is the ONLY thing required to define a base field and that the interface is what should be expected during alterations.

BaseFieldDefinition::createFromFieldStorageDefinition exists as a helper to create base fields from storage definitions. Perhaps we could create the same thing for FieldDefinitionInterface. However, given how there aren't many concrete examples of people actually using this flexibility, it might not be worth it.

The "special casing" in EntityFieldManager seems like it's mostly providing some defaults to BaseFieldDefinitions. I think that's a good enough reason by itself to add setters to the interface, given I don't think it's known yet that an alternative implementation would require such defaults in the first place.

So here is a proof of concept for essentially a "wont fix" approach.

The alternative I see is extra setters on FieldDefinitionInterface but that could possibly compromise the flexibility of fields coming from a source this is completely immutable. A field definition could start its life out as an instance of CantBeChangedFieldDefinition and something could replace it with an instance of FancyMutableBaseField and while they are both implementing FieldDefinitionInterface (and other code expects this), things could continue to work.

+++ b/core/lib/Drupal/Core/Entity/FieldableEntityInterface.php
@@ -33,6 +33,11 @@
+   * If you return fields as an instance of BaseFieldDefinition, the field name,
+   * target entity type ID and provider will be set automatically. If some other
+   * implementation of FieldDefinitionInterface is returned, this information
+   * must be explicitly available.

This is the part of the patch which I think best addresses the concerns which raised this issue in the first place, that is adding special casing for BaseFieldDefinition everywhere.

Instead of seeing the special casing as a required part of base fields, it'd essentially become a helper for that one type of implementation.

Re-rolling #22, fixes a couple of test failures by properly updating $fields array in language_entity_base_field_info_alter.
Also fixes BaseFieldDefinition::createFromFieldDefinition to only use methods available in FieldDefinitionInterface.

I think this would be a minor-only change. Since 8.9.x and 9.0.x are now in beta, I'm moving this to 9.1.x. Thanks!

I've re-rolled patch for 9.1

The patch failed for Drupal 9.2.x because an unused use statement in /core/modules/language/language.module, which hasn't be changed from the patch.

Thanks for the analysis! Tagging as novice as it sounds like a simple thing to deal with.

Re-roll against 9.2.x. Please review.

@Suresh We need a patch for Drupal 9.3.x.

Re-rolled patch for Drupal 9.3.x.

1. +++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
   @@ -94,6 +94,26 @@ public static function createFromFieldStorageDefinition(FieldStorageDefinitionIn
   +  public static function createFromFieldDefinition(FieldDefinitionInterface $definition) {
   

   Missing the parameter in the docblock.

2. +++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
   @@ -96,6 +96,29 @@ public static function createFromFieldStorageDefinition(FieldStorageDefinitionIn
   +  public static function createFromFieldDefinition(FieldDefinitionInterface $definition) {
   

   This new method is not used in core anywhere. At least we should add some testing for it.

Attached patch against Drupal 10.1.x

rerolled against 9.5.x

I converted patch #44 to a merge request and rerolled against 11.x

Note that from @daffie in #40, a test still needs to be added for the newly added BaseFieldDefinition::createFromFieldDefinition (the wrong function was referenced in the comment above, as createFromFieldStorageDefinition is actually used in core)

Adding credits from @tstoeckler's triage credit in #15.

And adding credit from the triage in #4.