Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info()

Is this still a thing?

Drupal\entity_test\FieldStorageDefinition has a @todo about this: https://api.drupal.org/api/drupal/core%21modules%21system%21tests%21modu...

Just hit this. Yes, it's still a thing. It's impossible to use hook_entity_bundle_field_info() in any sensible way without this.

No patch here AFAICT

Tagging for the rocket ship

Just ran into this, still needed.

If we introduce #2935932: Add a FieldDefinition class for defining bundle fields in code., we'll have builders that provide implementations of FieldStorageDefinitionInterface for both base fields and bundle fields.

What is the use case for defining field storage that isn't associated with configuration based fields or fields defined in code using those builders?

The issue title doesn't make sense we already have FieldStorageDefinition. We might want to "provide proper FieldStorageDefinition class"?
FieldDefinition class doesn't exist in 8.7.x codebase so can we please update the issue summary as well?

The issue title doesn't make sense we already have FieldStorageDefinition.

Do we?

I meant \Drupal\entity_test\FieldStorageDefinition. :)

This issue is not about the test class, indeed.

Now #2935932: Add a FieldDefinition class for defining bundle fields in code. is adding the FieldDefinition class so is this blocking that or not?

I think this issue was created before #2315237: Rename FieldDefinition to BaseFieldDefinition and the FieldDefinition mentioned in IS is now called BaseFieldDefinition. Is this correct?

Looking at the timeline of the issues, yes, that's correct.

As I am working with #2935932, this issue is now blocking that one for creating a FieldStorageDefinition to use in the new FieldDefinition::createFromFieldStorageDefinition().

I have created an initial patch that allows me to build my project using the latest patch in #2935932, but it still needs work as I have just used the one that was in entity_test.

This will end up as:

class FieldStorageDefinition extends ListDataDefinition implements FieldStorageDefinitionInterface, RequiredFieldStorageDefinitionInterface {}

So BaseFieldDefinition probably should extend.

+++ core/lib/Drupal/Core/Field/FieldStorageDefinition.php	(date 1532425628000)
@@ -0,0 +1,24 @@
+class FieldStorageDefinition extends BaseFieldDefinition {

This is not correct this should be other way around as per IS.

Alterations to BaseFieldDefinition should not be in scope here. Since a base field definition is both a FieldDefinitionInterface and a FieldStorageDefinitionInterface, it probably makes more sense for a base field to be composed of these two classes, not extending either one of them.

That sounds potentially disruptive however, so it should be explored thoroughly in a follow up imo.

Updating IS as I understand it.

With #2935932: Add a FieldDefinition class for defining bundle fields in code. in, we should really prioritize to this issue, as currently the DX of adding a bundle field via code is possibly even more wtf than before :)

Yup, I think we should start by writing a proper summary.

I started writing a patch for this, can probably clean it up this week.

Awesome, thanks @Sam152!

An IS update would be welcome, of course :)

First pass of this.

I don't really understand what needs to be changed in the IS? If you see something missing @jibran, just add it.

IS summary is fine for now.

1. +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinition.php
   @@ -0,0 +1,411 @@
   +    $field_definition = new static([]);
   +    $field_definition->type = $type;
   +    $field_definition->itemDefinition = FieldItemDataDefinition::create($field_definition);
   

   This is not $field_definition it is $field_storage_definition. Also FieldItemDataDefinition::create() doc block needs update.

2. +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinition.php
   @@ -0,0 +1,411 @@
   +    @trigger_error('FieldStorageDefinition::isQueryable() is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Instead, you should use \Drupal\Core\Field\BaseFieldDefinition::hasCustomStorage(). See https://www.drupal.org/node/2856563.', E_USER_DEPRECATED);
   

   Adding this to newly added method seems so wired.

3. +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinition.php
   @@ -0,0 +1,411 @@
   +    if (!isset($this->propertyDefinitions)) {
   ...
   +    if (isset($this->propertyDefinitions[$name])) {
   +      return $this->propertyDefinitions[$name];
   ...
   +    if (!isset($this->propertyDefinitions)) {
   ...
   +    return $this->propertyDefinitions;
   

   $propertyDefinitions doesn't exist for this class. We need to add this property. We also need __sleep() method for this class to unset $propertyDefinitions

4. +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinition.php
   @@ -0,0 +1,411 @@
   +    return !empty($this->definition['base_field']);
   

   This should always be FALSE.

5. +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinition.php
   @@ -0,0 +1,411 @@
   +  public function setBaseField($base_field) {
   

   This should not exist. If you are adding a basefield then use BaseFieldDefinition

Thanks for the review.

1. I think we actually have a bigger problem here. I think we actually need to pass it an instance of FieldDefinitionInterface or find a way to make it useable with only a storage definition. For example it has methods that contain code such as:

$this->fieldDefinition->getFieldStorageDefinition()->getPropertyDefinition($name);

Passing in and assigning a storage definition to the fieldDefinition will cause exceptions. That's probably not that bad given that it's an internal property and the only calls that should be made are the ones that happen within FieldStorageDefinition itself, but it seems like we should avoid that if possible. For now, experimenting with passing it a new instance of FieldDefinition instead.

2. Yeah, not sure how to avoid that.
3. Addressed this and added coverage for __sleep and __clone. Should have tests to prove why both properties should be removed during __sleep.
4/5. Why? You could use this builder class along with FieldDefinition for satisfying the needs of hook_entity_field_storage_info and hook_entity_base_field_info.

Still looking at test fails, and there is one @todo left.

Fixing a chunk of the tests, but I believe this exposes a bug in EntityDefinitionUpdateManager and FieldStorageDefinitionListener::onFieldStorageDefinitionDelete. Only storage definitions are detected and gathered by the updated manager, so unless a storage definition also implements FieldDefinitionInterface, a bundle field definition will never get marked for purging. From what I can see, ::onFieldStorageDefinitionDelete essentially contains a hack to add purging for field definitions when it's dealing with a storage object that is a composite.

try {
  $storage_definition_has_mutable_delete_flag = $storage_definition instanceof BaseFieldDefinition || $storage_definition instanceof FieldStorageDefinition;
  if ($storage_definition_has_mutable_delete_flag && $storage instanceof FieldableEntityStorageInterface && $storage->countFieldData($storage_definition, TRUE)) {
    $deleted_storage_definition = clone $storage_definition;
    $deleted_storage_definition->setDeleted(TRUE);
    $this->deletedFieldsRepository->addFieldStorageDefinition($deleted_storage_definition);

    if ($deleted_storage_definition instanceof FieldDefinitionInterface) {
      $this->deletedFieldsRepository->addFieldDefinition($deleted_storage_definition);
    }
  }
}

It would be great if @amateescu or @plach could verify this or help explain what is going on.

Edit: Thinking about this some more, I think the assertion in the test should be replaced with a @todo that points to an issue that adds support for purging stand-alone field definitions. Since it looks like that functionality doesn't actually exist right now for bundle fields, I don't think we'd actually be breaking anything, just replacing some faulty assertions with an issue-reference.

Fixing final @todo.

Just a minor review because I happened to be working with BFDs just now and wanted to compare their behavior.

1. +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinition.php
   @@ -0,0 +1,441 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getSchema() {
   +    if (!isset($this->schema)) {
   

   You left out the declaration of $schema, which is present in BFD and seems to be needed here.

2. +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinition.php
   @@ -0,0 +1,441 @@
   +      $this->schema = $schema;
   

   BFD has a notion of custom indexes here. Why was this behavior removed?

1. Good point, we should add that. Probably should also unset it in __sleep.
2. Good question. I couldn't actually see how these custom indexes were set in BFD, so couldn't test for their behaviour. It's actually on a list of follow-ups I have to remove that code from BFD, since it seems like it's dead code to me.

Only storage definitions are detected and gathered by the updated manager, so unless a storage definition also implements FieldDefinitionInterface, a bundle field definition will never get marked for purging.

So.. this patch changes an important assumption that was previously made about "bundle fields", which is that they also implement FieldStorageDefinitionInterface. Even though that assumption is not correct, the contrib Entity module and probably many other instances in custom code rely on the fact that bundle field definitions are now purgeable because of that.

So my opinion is that we should first do that issue that adds support for purging stand-alone field definitions so we don't impact existing code as much as possible, and then continue with this one.

we should first do that issue that adds support for purging stand-alone field definitions

Added #3016255: [PP-1] Move adding fields to the DeletedFieldsRepository to FieldDefinitionListener, so uninstalled field definitions are purged automatically for this.

I think we actually have two issues we can tackle here, one which should be a blocker and one which shouldn't. I would split this as:

1.

Field definition purging should be delegated to \Drupal\Core\Field\FieldDefinitionListener.

I think anyone manually dealing with adding/removing field definitions is already required to call this listener to ensure field maps are added etc. Even the contrib implementation doesn't use auto-magical installation/uninstallation of these definitions using the blanket update manager. So, this would be centralising that logic and we could ensure that anywhere that manually adds definitions to the DeletedFieldsRepository would instead call the listener and at the same time all of the field maps would be cleaned up (I think this is potentially a bug and blind-spot in the current implementation, but I haven't checked).

Our test case would then be updated to call \Drupal\Core\Field\FieldDefinitionListener::onFieldDefinitionDelete, purging would kick in and the test would pass, existing implementations which call this method would continue benefit from purging.

2.

The EntityDefinitionUpdateManager should automatically detect and apply updates to changed FieldDefinition items.

This to me is the new feature and should not be a blocker, especially with the conversations about automatic entity definition updates being discouraged.

@amateescu let me know what you think of that plan or if I've missed any key details.

Or, reading #46.1 again, should EntityDefinitionUpdateManager have methods for installing both FieldStorageDefinition instances and FieldDefinition instances, those methods could then delegate to the listener, and the listener could add field purging. That would essentially create the same DX for managing the installation of storage definitions and field definitions.

Essentially all the update manager does is delegate to the listeners, so I'm not sure why the contrib module chose to call those directly? Should they instead be using EntityDefinitionUpdateManager as the entry point for all changes?

I think the point still stands though, that automatic updates wouldn't have to block this.

So maybe this is three issues:

1. EntityDefinitionUpdateManager should provide a way to install/uninstall FieldDefinition items.
2. Field purging for field definitions should be kicked-off by FieldDefinitionListener
3. EntityDefinitionUpdateManager should automatically detect and apply updates to changed field definitions.

So, currently field definitions aren't in the installed schema repository, so we can't access the original field definition for the purposes of satisfying calling:

\Drupal\Core\Field\FieldDefinitionListenerInterface::onFieldDefinitionUpdate($field_definition, $original);

from:

\Drupal\Core\Entity\EntityDefinitionUpdateManager::updateFieldDefinition(FieldDefinitionInterface $field_definition)

So I think perhaps this is blocked by doing everything at the same time, not sure they can reasonably be split up.

Lets use #3016255 for moving field purging to the field listener and #3018110 for the foundational work required to actually detect and run updates on installed field definitions.

So we're blocked behind:

• #3016255: [PP-1] Move adding fields to the DeletedFieldsRepository to FieldDefinitionListener, so uninstalled field definitions are purged automatically
• #3018110: Store stand-alone field definitions in the EntityLastInstalledSchemaRepository, detect changes and allow changes to field definitions to be installed/updated/uninstalled in EntityDefinitionUpdateManager

I looked at this and the two other issues mentioned in #49 today, and here's how I think we should proceed here:

- #3018110: Store stand-alone field definitions in the EntityLastInstalledSchemaRepository, detect changes and allow changes to field definitions to be installed/updated/uninstalled in EntityDefinitionUpdateManager is not really a requirement IMO. We store entity type and field storage definitions and the "last installed/schema" repositories because those objects have a direct impact on the actual storage layer (the database), while field definitions don't have any interactions with the storage.

- I don't think we want to introduce CRUD methods for field definitions in EntityDefinitionUpdateManager. We should simply call FieldDefinitionListener::onFieldDefinition(Create|Update|Delete)() when needed, just like we do with EntityBundleListenerInterface::onBundle(Create|Delete)() which must be called when you add or delete a bundle.

The attached interdiff is about half way implementing #3016255: [PP-1] Move adding fields to the DeletedFieldsRepository to FieldDefinitionListener, so uninstalled field definitions are purged automatically, which seems easier to do here rather than in a separate issue. What's needed for it work properly is to introduce the concept of "deleted" to FieldDefinitionInterface, (i.e. FieldDefinitionInterface::isDeleted() and FieldDefinition::setDeleted()), which would allow us to try and retrieve the field storage definition in \Drupal\Core\Field\FieldDefinition::getFieldStorageDefinition() from the deleted fields repository first, like we do for configurable fields (\Drupal\field\Entity\FieldConfig::getFieldStorageDefinition()).

Apart from that, I think FieldDefinition::createFromFieldStorageDefinition() really needs a bundle argument, otherwise the object you receive after calling that method is mostly useless. And forcing developers to learn that they *must* call setTargetBundle() in order to have a usable object is not very DX-friendly. The same point applies to FieldStorageDefinition::create(), it needs a field_name and target_entity_id parameters for the same reason.

Thanks for writing this up. When you proposed making them purgeable, I thought you meant they needed the same DX as purged composite definitions, where, since the storage definition was being removed at the same time, they got automatic purging. Requiring calls to FieldDefinitionListener seems totally reasonable to me, so we can close #3018110: Store stand-alone field definitions in the EntityLastInstalledSchemaRepository, detect changes and allow changes to field definitions to be installed/updated/uninstalled in EntityDefinitionUpdateManager.

With regards to the $bundle argument, I'm not sure that works because you can also in theory use FieldDefinition for the definition component of a base field, or it could be used internally by a composite to represent the definition component of a base field.

I'll look at moving this forward with with the interdiff from #50.

Some work on this.

1. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
   @@ -263,9 +268,29 @@ public function setFieldStorageDefinition(FieldStorageDefinitionInterface $stora
   +    $deleted_storage_definitions = $this->getDeletedFieldsRepository()->getFieldStorageDefinitions();
   +    // If the storage definition associated with this field definition has been
   +    // deleted, ensure we return the deleted version, so when deleting and
   +    // purging a field definition, items are purged from the deleted storage.
   +    if (isset($deleted_storage_definitions[$this->fieldStorageDefinition->getUniqueStorageIdentifier()])) {
   +      return $deleted_storage_definitions[$this->fieldStorageDefinition->getUniqueStorageIdentifier()];
   +    }
   

   How about this for an approach? It seems we can infer pretty easily if a field definition belongs to some storage that has been deleted.

   Alternatively we could simply add this logic to \Drupal\Core\Entity\Sql\SqlContentEntityStorage::readFieldItemsToPurge as well, that seems to be the key part that needs this information. From the tests I ran, doing that would also be an approach to making this go green.

2. +++ b/core/lib/Drupal/Core/Field/FieldDefinitionListener.php
   @@ -142,11 +142,6 @@ public function onFieldDefinitionDelete(FieldDefinitionInterface $field_definiti
   -
   -      // Also delete the associated field storage definition since it is not
   -      // used anymore.
   -      $storage_definition = $field_definition->getFieldStorageDefinition();
   -      \Drupal::service('field_storage_definition.listener')->onFieldStorageDefinitionDelete($storage_definition);
   

   I couldn't see why we would do this? Presumably we would want removing storage definitions to be more explicit and go through the normal uninstallation. Imagine removing a field from bundle X and adding it to bundle Y in the same update?

Some test fixes.

Fixing the last failing tests.

+  public function isQueryable() {
+    @trigger_error('FieldStorageDefinition::isQueryable() is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Instead, you should use \Drupal\Core\Field\BaseFieldDefinition::hasCustomStorage(). See https://www.drupal.org/node/2856563.', E_USER_DEPRECATED);
+    return !$this->hasCustomStorage();

Doesn't the FieldStorageDefinition class have its own hasCustomStorage() method? The deprecation message should not suggest to use \Drupal\Core\Field\BaseFieldDefinition::hasCustomStorage(). If that is the case, then also is deprecated in Drupal 8.4.0 should be changed.

You're right, updating the deprecation message.

The patch was green other than the doc issue. What else is remaining here other than change notice?

Just a follow up review from @amateescu based on #50/#52.

Fixing the test case that failed in #59.

-  public function __construct(EntityTypeManagerInterface $entity_type_manager, EntityFieldManagerInterface $entity_field_manager, KeyValueFactoryInterface $key_value_factory, CacheBackendInterface $cache_backend) {
+  public function __construct(EntityTypeManagerInterface $entity_type_manager, EntityFieldManagerInterface $entity_field_manager, KeyValueFactoryInterface $key_value_factory, CacheBackendInterface $cache_backend, DeletedFieldsRepositoryInterface $deleted_fields_repository) {
     $this->entityTypeManager = $entity_type_manager;
     $this->entityFieldManager = $entity_field_manager;
     $this->keyValueFactory = $key_value_factory;
     $this->cacheBackend = $cache_backend;
+    $this->deletedFieldsRepository = $deleted_fields_repository;

As I have seen done from similar patches, the added parameter should get a default value (NULL), and the code to initialize $this->deletedFieldsRepository should use the passed argument, or take the service using the \Drupal class.

+  public function isQueryable() {
+    @trigger_error('FieldStorageDefinition::isQueryable() is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Instead, you should use FieldStorageDefinition::hasCustomStorage(). See https://www.drupal.org/node/2856563.', E_USER_DEPRECATED);
+    return !$this->hasCustomStorage();
+  }

The trigger message should say deprecated in Drupal 8.7.0, since the code has not been changed in Drupal 8.4.0, but hopefully will be committed before Drupal 8.7.0 is released. If the patch goes in Drupal 8.8.x. the message will say deprecated in Drupal 8.8.0.

Constructor signatures are not APIs and FieldDefinitionListener doesn't strike me as a class that should be extended, so I think the current implementation is fine.

Good point re: the message. Unless someone else gets to updating the message, I'll wait for any architectural feedback that might come through and try to address it all in one go.

See what done in EntityDefinitionUpdateManager.php, in the class constructor.

  public function __construct(EntityManagerInterface $entity_manager, EntityLastInstalledSchemaRepositoryInterface $entity_last_installed_schema_repository = NULL) {
    $this->entityManager = $entity_manager;
    if (!isset($entity_last_installed_schema_repository)) {
      @trigger_error('The $entity_last_installed_schema_repository parameter was added in Drupal 8.6.x and will be required in 9.0.0. See https://www.drupal.org/node/2973262.', E_USER_DEPRECATED);
      $entity_last_installed_schema_repository = \Drupal::service('entity.last_installed_schema.repository');
    }
    $this->entityLastInstalledSchemaRepository = $entity_last_installed_schema_repository;
  }

The last parameter was added in 8.6.x, and it got a default value.

This patch is changing a service too, so I take the code should be changed in the same way.

I don't know why that change was made so defensively, there could be an element of committer discretion there when evaluating how disruptive a constructor change is, but these are not officially APIs: https://www.drupal.org/core/d8-bc-policy#constructors, especially imo for things which aren't targeted for a patch release.

NR for the review I mentioned in #62.

I think for the change record, we should also simply draft an update to: https://www.drupal.org/node/2982512. Both of these are quite related, so it would be good to keep it as a reference for what a custom implementation of the field hooks with FD/FSD looks like.

@jibran asked me to look at this. I hadn't seen this issue before. I think @amateescu should definitely review this before it goes in — AFAICT he has largely nudged this patch in this direction. He knows far more about this than I do for sure :)

1. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
   @@ -44,6 +44,11 @@ class FieldDefinition extends ListDataDefinition implements FieldDefinitionInter
   +  /**
   +   * @var \Drupal\Core\Field\DeletedFieldsRepositoryInterface
   +   */
   +  protected $deletedFieldsRepository;
   

   Nit: incomplete docs.

2. +++ b/core/lib/Drupal/Core/Field/FieldDefinitionListener.php
   @@ -118,6 +129,14 @@ public function onFieldDefinitionDelete(FieldDefinitionInterface $field_definiti
   +    $deleted_field_definitions = $this->deletedFieldsRepository->getFieldDefinitions();
   +    if ($field_has_data && !isset($deleted_field_definitions[$field_definition->getUniqueIdentifier()])) {
   +      $this->deletedFieldsRepository->addFieldDefinition($field_definition);
   +    }
   

   🤔 In what circumstances could it happen that !isset($deleted_field_definitions[$field_definition->getUniqueIdentifier()]) evaluated to FALSE instead of TRUE?

   IOW: how could we re-delete something that was already deleted?

3. +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinition.php
   @@ -0,0 +1,441 @@
   +class FieldStorageDefinition extends ListDataDefinition implements FieldStorageDefinitionInterface, RequiredFieldStorageDefinitionInterface {
   

   🤔 I expected most of this logic to be moved from elsewhere, but that's not the case.

   It seems much of this is copy/pasted instead, from \Drupal\Core\Field\BaseFieldDefinition.

   Shouldn't this patch do BaseFieldDefinition extends FieldStorageDefinition? Why not?

Shouldn't this patch do BaseFieldDefinition extends FieldStorageDefinition? Why not?

That will be a follow up #3014760: [PP-2] Create an abstract composite field definition/storage definition and update BaseFieldDefinition to use it.

Here is a reroll of #63.

Re: #64, it actually was deprecated in 8.4, so the message makes sense to me, but updating it to reference the interface instead, because that's what we have to implement and what was actually deprecated.

Re: #70:

1. Good catch, fixed.
2. Good question, I can't think of any side effects here, lets see what the testbot says.
3. Right now I've focused efforts on pulling apart the definition and storage builders from the base field definition class. I think eventually you could have composites such as BaseFieldDefinition and maybe even BundleFieldDefinition use these internally, but there isn't obvious consensus on exactly how these should be reused yet. The main that is being solved is that these also address a big API deficiency unrelated to BaseFieldDefinition, so they are in my eyes worth building and integrating seperate to the more difficult reuse conversation. I don't know the full scope of the use they may eventually have, perhaps there is even some utility in Field(Storage)?Config land too.

Reroll + reverting #73.2 which I believe is what made the tests fail. Despite putting a debugger on this, I'm not quite sure why it was causing those tests to fail.

Reroll due to changes in #2554235: Make the content entity storage and entity query use the last installed definitions instead of the ones living in code

Reroll due to changes in #2554235: Make the content entity storage and entity query use the last installed definitions instead of the ones living in code

Made with git in correct format this time.

Additional file for 8.7.x

Apart from creating the patch, the fields are not being created in the database. I will try and investigate more.

Rerolling this.

Fixing deprecations.

He is the reroll and a change notice https://www.drupal.org/node/3076611. All the latest feedback has been addressed so setting it to RTBC because it is ready IMO.

PHPCS fixes.

Setting back to RTBC.

The user who provided the patch cannot change the status to R&TBC.

FWIW, I just rerolled the patch twice and fixed PHPCS fails once. @Sam152 created almost all the patch.

Yes, but you changed the status after adding your patch. The idea is that the patch should be reviewed from a user who didn't post the patch.

1. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
   @@ -62,21 +69,21 @@ public static function createFromFieldStorageDefinition(FieldStorageDefinitionIn
   -    return $this->getFieldStorageDefinition()->getName();
   +    return $this->fieldStorageDefinition->getName();
   ...
   -    return $this->getFieldStorageDefinition()->getType();
   +    return $this->fieldStorageDefinition->getType();
   ...
   -    return $this->getFieldStorageDefinition()->getTargetEntityTypeId();
   +    return $this->fieldStorageDefinition->getTargetEntityTypeId();
   
   @@ -263,9 +270,29 @@ public function setFieldStorageDefinition(FieldStorageDefinitionInterface $stora
      public function getFieldStorageDefinition() {
   +    $deleted_storage_definitions = $this->getDeletedFieldsRepository()->getFieldStorageDefinitions();
   +    // If the storage definition associated with this field definition has been
   +    // deleted, ensure we return the deleted version, so when deleting and
   +    // purging a field definition, items are purged from the deleted storage.
   +    if (isset($deleted_storage_definitions[$this->fieldStorageDefinition->getUniqueStorageIdentifier()])) {
   +      return $deleted_storage_definitions[$this->fieldStorageDefinition->getUniqueStorageIdentifier()];
   +    }
        return $this->fieldStorageDefinition;
   

   So now $this->fieldStorageDefinition might be different than what is being returned by :: getFieldStorageDefinition()??

2. +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinition.php
   @@ -0,0 +1,441 @@
   +  public function isBaseField() {
   +    return !empty($this->definition['base_field']);
   +  }
   ...
   +  public function setBaseField($base_field) {
   +    $this->definition['base_field'] = $base_field;
   +    return $this;
   

   How is it possible for a FieldStorageDefinition to become a base field and what does this mean? From the issue summary:

   We should add a dedicated field storage definition class which allows modules to define field storage definitions in hook_entity_field_storage_info() without having a config-field easily.

   So this basically means that a FieldStorageDefinition cannot be a base field.

3. Also now we have for a lot of the implemented methods in FieldStorageDefinition the same implementation. For example:

     public function setTranslatable($translatable) {
       $this->definition['translatable'] = $translatable;
       return $this;
     }
   

   has exactly the same body additionally in \Drupal\Core\Field\BaseFieldDefinition and in \Drupal\Core\Field\FieldDefinition. What about putting the method implementations in a trait instead?

Hey @hchonov, thanks for the review!

1. Definitely worth looking into this. I wonder if replacing access to $this->fieldStorageDefinition with calls to the method is instead preferable?
2. I think we should treat this flag "a storage definition associated with a base field" not as the complete definition, it's a member of FieldStorageDefinitionInterface after all. Base fields could use the object to store details about their storage internally in the future.
3. One of the proposed future clean ups is leveraging FieldDefinition and FieldStorageDefinition internally in the other builder objects. I don't think reusing the body of these relatively uncomplicated methods is worth is the additional surface area of a trait, which may become obsolete.

1.
Isn't it possible that the property holds the deleted storage definition instead? If the field storage definition is deleted, then what is currently the value of the property?

2.

Base fields could use the object to store details about their storage internally in the future.

But currently they don't and therefore I would say it is better to simply return FALSE. That might be the same for other methods - I haven't looked that deeply into the patch yet.

3.
I just like to follow the rule of three :). I guess that we could define the trait as internal and if it becomes obsolete, then simply remove it.

Re: #2, the return value of hook_entity_base_field_info is FieldDefinitionInterface[]. I believe you could feasibly implement base fields with both FieldDefinition and FieldStorageDefinition. I think if these builder objects are considered kind of "primitives" of the field system, they should represent the matrix of possible return values from the existing APIs and hooks.

No idea why, but based on the history of this issue and my testing, \Drupal\KernelTests\Core\Entity\EntityDefinitionUpdateTest fails when implementing #91.1.

Just for reference if you're purely making some CS fixes it's fine to mark the patch RTBC at the same time - we often have to make minor DrupalCS fixes on commit (which is more prone to risk than a self-RTBC).

@hchonov, #91:

1: I agree that this is definitely non ideal. In fact having a domain object depending on a service feels rather backwards. I'm wondering whether we could avoid messing with the object runtime values and rather altering them in EntityFieldManager, forcing the deleted status there.

2:

How is it possible for a FieldStorageDefinition to become a base field and what does this mean?

I think that’s ok, because one thing that has been implied in all these discussions is that FD and FSD are two aspects of defining a Field, and a Field can be either Base or Bundle: so it’s perfectly ok for a FSD to know/be able to tell whether it’s defining a Base or Bundle field. Actually it’s critical, given how that impacts our default storage logic, for instance.

Conceptually it should be possible to define a Base field via two separate FD and FSD objects and things should behave normally. I’m pretty sure something would break in practice because our codebase is riddled with instanceof BaseFieldDefinition checks. However ::isBaseField() === TRUE shouldn’t automatically imply that you’re dealing with a BaseFieldDefinition instance.

We should probably audit the various instanceof occurrences and verify whether they still make sense or can be removed, or at least relaxed. I think in many cases they were meant to ensure that setters were available rather than checking whether the defined field was a base field, but we should definitely verify this on a case by case basis.

3: That's a very good point: from a maintainability perspective I think an internal trait encapsulating that logic would help ensuring that any change in the logic is applied to all places needing it. OTOH if we introduce a trait, people might start using it, even if it's marked as @internal, and once we remove it we might cause contrib/custom code to break, which of course would be its fault but still... Given the duplicated code is relatively simple and we already have a plan to clean that up, I think I'm fine with the duplication for now.

I looked at the code but didn't perform a full review yet (I'm missing test changes), however I have one concern about the current patch:

+++ b/core/lib/Drupal/Core/Field/FieldStorageDefinition.php
@@ -0,0 +1,441 @@
+    $storage_definition->itemDefinition = FieldItemDataDefinition::create(FieldDefinition::createFromFieldStorageDefinition($storage_definition));

This is unfortunate: this method introduces a dependency on FD just because it needs to instantiate a FieldItemDataDefinition. I’m wondering whether there’s any workaround possible because it seems that’s only a storage for some settings, so maybe we can add a separate FieldStorageDataDefinition and decouple FSD from FD. Didn't have time to look closely, so this might make no sense, but I'd be happier if there were a way to avoid adding this (almost circular) dependency :)

Briefly discussed my concern above with @amateescu and we came up with the attached proposal. Thoughts?

Aside from that and #97.1, the patch looks great to me, I found only these minor issues:

1. +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinition.php
   @@ -0,0 +1,441 @@
   +    // Assign settings individually, in order to keep the current values
   +    // of settings not specified in $settings.
   

   This does not wrap at 80 chars.

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php
   @@ -645,7 +646,7 @@ public function testBundleFieldDeleteWithExistingData() {
        // Save an entity with the bundle field populated.
        entity_test_create_bundle('custom');
   -    $entity = $storage->create(['type' => 'test_bundle', 'new_bundle_field' => 'foo']);
   

   The entity_test_create_bundle() call can be removed.

Thanks for looking at this @plach :)

I'm not sure I have capacity to see this one out to the end, so please feel free to post full patches and take it in any direction that can gain consensus.

Reroll the patch with interdiff from #98. This addresses #98. #91.1 is still pending.

In fact having a domain object depending on a service feels rather backwards. I'm wondering whether we could avoid messing with the object runtime values and rather altering them in EntityFieldManager, forcing the deleted status there.

This seems worth investigating, the manager would detect if a definition belongs to deleted storage and set the deleted storage before returning.

Would something like this make sense?

PS: This is failing.

Thanks @jibran, this is almost exactly what I had in mind!

1. +++ b/core/lib/Drupal/Core/Entity/EntityFieldManager.php
   @@ -449,6 +457,7 @@ public function getFieldStorageDefinitions($entity_type_id) {
          $this->fieldStorageDefinitions[$entity_type_id] += $field_storage_definitions;
   +      $this->fieldStorageDefinitions[$entity_type_id] +=  $this->getDeletedFieldStorageDefinitions($entity_type_id);
   

   I think this won't always work because deleted definitions collected in $field_storage_definitions will take precedence over the ones stored in the deleted repository.

   I think we should just update $field_storage_definitions by marking definitions as deleted when needed.

   This should probably happen before caching them, btw.

2. +++ b/core/lib/Drupal/Core/Entity/EntityFieldManager.php
   @@ -657,4 +666,25 @@ public function getExtraFields($entity_type_id, $bundle) {
   +    $this->deletedFieldsRepository = \Drupal::service('entity_field.deleted_fields_repository');
   

   We are going to inject this in the final version of the patch, right? :)

3. +++ b/core/lib/Drupal/Core/Entity/EntityFieldManager.php
   @@ -657,4 +666,25 @@ public function getExtraFields($entity_type_id, $bundle) {
   +      return $field_definitions;
   

   Wrong indentation :)

4. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
   @@ -69,21 +62,21 @@ public static function createFromFieldStorageDefinition(FieldStorageDefinitionIn
   -    return $this->fieldStorageDefinition->getName();
   +    return $this->getFieldStorageDefinition()->getName();
   ...
   -    return $this->fieldStorageDefinition->getType();
   +    return $this->getFieldStorageDefinition()->getType();
   ...
   -    return $this->fieldStorageDefinition->getTargetEntityTypeId();
   +    return $this->getFieldStorageDefinition()->getTargetEntityTypeId();
   

   I'd revert these, saving a method call may have a positive impact when dealing with many definitions.

Thanks, the review is really helpful.

1. Great I'll do that, I was not sure $field_storage_definitions will have deleted values in it.
2. Of course, it was just POC I spun up with the help from @Sam152.
3. Makes sense.

+++ b/core/lib/Drupal/Core/Field/FieldStorageDefinitionInterface.php
@@ -351,4 +351,14 @@ public function getUniqueStorageIdentifier();
+  /**
+   * Sets whether the field storage is deleted.
+   *
+   * @param bool $deleted
+   *   Whether the field storage is deleted.
+   *
+   * @return $this
+   */
+  public function setDeleted($deleted);
+
 }

What do think about adding it to interface? It seems wired that FieldStorageConfig can be marked as deleted.

I was not sure $field_storage_definitions will have deleted values in it.

Me neither, OTOH if they're not available I'm curious to see what would still require them.

What do think about adding it to interface? It seems wired that FieldStorageConfig can be marked as deleted.

That's definitely ok: we're bringing more consistency to the API.

Ah, sorry, I get what you mean: setters are not part of the API normally. We could either keep that or we might check whether the definition has the setDeleted() method. Ultimately we should move to a proper builder approach where the EFM always gets mutable objects and stores immutable objects derived by those, but we're not there yet.

While fixing the fail \Drupal\KernelTests\Core\Entity\EntityDefinitionUpdateTest::testBundleFieldDeleteWithExistingData I realized \Drupal\Core\Field\DeletedFieldsRepository::addFieldStorageDefinition adds deleted storage definition but doesn't update the corrisponding field definitions which are already marked for deletion and have stale storage definition object so I went down this path. Following patch address #91.1 and interdiff is from #100.

Let's simplify \Drupal\field\Entity\FieldConfig::getFieldStorageDefinition() as well.

I reverted changes in \Drupal\field\Entity\FieldConfig::getFieldStorageDefinition() it was either that or I hack field_update_8500() and store storage definition for corrisponding field definitions.

I have updated the patch in #111 to work with Drupal 8.7.8.

I spent some time looking at this patch, and I think it's trying to fix #2346347: Finalize API for creating, overriding, and altering code-defined bundle fields without actually fixing it because a field storage definition is not a "bundle field" :)

IMO, a bundle field should be an object with two class members: $fieldDefinitions = FieldDefinition[] (an array of field definition objects keyed by bundle name) and $fieldStorageDefinition = FieldStorageDefinition (a field storage definition object).

So my proposal is to go with a very simple patch here which just adds the new FieldStorageDefinition class and its test, and discuss bundle fields properly in the issue mentioned above.

Here's a patch which removes all the extra fluff from #111.

1. +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinition.php
   @@ -0,0 +1,445 @@
   +      $definition = \Drupal::service('plugin.manager.field.field_type')->getDefinition($this->getType());
   +      $class = $definition['class'];
   

   We can use $this->getItemClass() here.

2. +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinition.php
   @@ -0,0 +1,445 @@
   +    unset($vars['propertyDefinitions'], $vars['typedDataManager'], $vars['fieldTypeManager']);
   

   This object doesn't have a typedDataManager member (anymore) :)

+++ /dev/null
@@ -1,25 +0,0 @@
-/**
- * A custom field storage definition class.
- *
- * For convenience we extend from BaseFieldDefinition although this should not
- * implement FieldDefinitionInterface.
- *
- * @todo Provide and make use of a proper FieldStorageDefinition class instead:
- *   https://www.drupal.org/node/2280639.
- */
-class FieldStorageDefinition extends BaseFieldDefinition {

FWIW, none of the changes I implemented originally were done with the intention of having an opinion on how bundle fields should work. It was the bare minimum number of changes required to replace the test FSD class in entity_test with the new one introduced in this issue. I think there were a number of assumptions exposed, where tests or core inadvertently required the test class instead of just an implementation of FieldStorageDefinitionInterface.

+++ b/core/lib/Drupal/Core/Field/FieldStorageDefinition.php
@@ -0,0 +1,445 @@
+    $storage_definition->fieldTypeManager = \Drupal::service('plugin.manager.field.field_type');

Unfortunately this won't work, as serialization unsets the reference. We'll need the usual ::fieldTypeManager() method lazily loading the service :(

@Sam152, #116:

Makes sense, let's try to restore the test changes required to use the new class: I can work on that myself if none beats me to it.

huh! #114 seems anticlimatic. I thought #111 was really close but thank you @amateescu looking into it. I really appricate that.

Same as #116 and #118 I think we should have at least functional test for the class to see how it would intract with whole system so setting it to NW.

@plach and I just discussed this issue at length and we came up with a plan to implement the full "bundle field" approach. I'll try to work on it ASAP :)

Here's a short summary of what we agreed on:

• config fields have setters → they are builder classes
• base fields have setters → they are builder classes
• any other field returned by hook_entity_field_*_info() / _alter() is assumed to be a builder class implementing the mutable interface exposing setters
• the core system provides a definition that’s immutable and is built from the ones passed around
for this to work we might have to cleanup some instanceofs
this should be BC for hook implementers
and API consumers should not modify definitions at runtime
and the immutable class could be this wrapper around FD + FSD

Reading #120, it's not clear if this issue fits into that picture? It seems like it kind of does as a primitive of the broader API?

I'll try to work on it ASAP

Any progress here? I'm happy to help here and coordinate in slack if that's possible.

The one in the test should be removed and replaced with the new one: https://api.drupal.org/api/drupal/core%21modules%21system%21tests%21modu...

And switching the one in the test shows that there's something wrong with the storage class in the patch:

1) Drupal\KernelTests\Core\Entity\EntityBundleFieldTest::testCustomBundleFieldUsage
Custom field table was deleted
Failed asserting that true is false.

/var/www/docroot/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit6/TestCompatibilityTrait.php:34
/var/www/docroot/core/tests/Drupal/KernelTests/Core/Entity/EntityBundleFieldTest.php:112

The test failure is this line:

    $table = $table_mapping->getDedicatedDataTableName($entity->getFieldDefinition('custom_bundle_field')->getFieldStorageDefinition(), TRUE);

  ...

    $this->assertFalse($this->database->schema()->tableExists($table), 'Custom field table was deleted');

which debugging shows is checking the *deleted field data* table, because of the TRUE parameter to getDedicatedDataTableName(). That's the table 'field_deleted_data_1e1de71614' in this case.

The actual field tables are gone -- doing this passes:

    $table = $table_mapping->getDedicatedDataTableName($entity->getFieldDefinition('custom_bundle_field')->getFieldStorageDefinition());
    $this->assertFalse($this->database->schema()->tableExists($table), 'Custom field table was deleted');

AFAICT the problem is that FieldStorageDefinitionListener::onFieldStorageDefinitionDelete() isn't adding the field to its stored list of deleted fields, which then means that field_purge_batch() doesn't see it.

OK here's a stab at making core/tests/Drupal/KernelTests/Core/Entity/EntityBundleFieldTest.php pass.

I'm skipping over comments #116 to #122, sorry... too much of rabbithole :( I just need something that works for the moment!

Here are my changes:

- reroll of #114 on 8.9.x
- changes requested in #115
- change entity_schema_test.module to use the new FieldStorageDefinition rather than the test one (and delete the test one)
- changed FieldStorageDefinitionListener so the test passes.

Fixed the test failures on the last patch that are due to the class in test module being removed.

Latest patch was crashing when in actual use editing an entity with bundle fields.

First of all, the tests weren't picking this up at all! We need functional tests for entities with bundle fields.

Second, this in FieldStorageDefinition doesn't seem to get called:

  public static function create($type) {
    $storage_definition = new static([]);
    $storage_definition->type = $type;
    // Create a definition for the items, and initialize it with the default
    // settings for the field type.
    $storage_definition->fieldTypeManager = \Drupal::service('plugin.manager.field.field_type');
    $default_settings = $storage_definition->fieldTypeManager->getDefaultStorageSettings($type);
    $storage_definition->setSettings($default_settings);
    return $storage_definition;
  }

This is what was causing the crash -- fieldTypeManager sometimes isn't set on the FieldStorageDefinition, and I can't figure out why!

I'm changing it to call the service directly with \Drupal::service(), which BaseFieldDefinition does too, so I figure that's acceptable.

ARGH. I keep missing some mentions of the old test class.

> I'm changing it to call the service directly with \Drupal::service(), which BaseFieldDefinition does too, so I figure that's acceptable.

Done this too in the latest patch.

I'm rather confused by the remaining test failure:

> 1) Drupal\KernelTests\Core\Entity\EntityDefinitionUpdateTest::testBundleFieldDeleteWithExistingData
> Error: Call to undefined method Drupal\Core\TypedData\Plugin\DataType\Any::setLangcode()

I've been trying to do some digging.

During the test, the 'any' data type (which I didn't even know existed) gets set here in DataDefinition:

  public function getDataType() {
    return !empty($this->definition['type']) ? $this->definition['type'] : 'any';
  }

Why is $this->definition['type'] empty here? That doesn't seem right.

FieldStorageDefinition::create() doesn't set it in the settings array:

  public static function create($type) {
    $storage_definition = new static([]);
    $storage_definition->type = $type;
    // Create a definition for the items, and initialize it with the default
    // settings for the field type.
    $default_settings = \Drupal::service('plugin.manager.field.field_type')->getDefaultStorageSettings($type);
    $storage_definition->setSettings($default_settings);
    return $storage_definition;
  }

But is that incorrect? BaseFieldDefinition doens't do it either, and outputting the field definitions of base fields shows that ->definition['type'] isn't set either.

So why is getDataType() getting called? I'm confused. Going to leave this here for now, I'm afraid; I don't think I can go further down this rabbithole in my project.

Adding the needs tests tag because I think we need functional tests that show an entity with bundle fields being loaded and edited in a form -- see #129.

Please add the interdiffs as well.

Tweaked #131 to work on 9.1.x. May well work on other versions too.

DrupalCI is failing.

Fixed the circleCi error in #139 patch, thanks!

The test fails because an outdated schema and because the following error.

Call to undefined method Drupal\Core\TypedData\Plugin\DataType\Any::setLangcode()

Seems to have a few test failures.

Hello!

I'd like to share my patch, aimed for Drupal 10.1.x. It's based on patch 143 and fixes one apllication conflict.

Changes

• Removed "Drupal\Core\Render\Element" lib on patch's target "core/lib/Drupal/Core/Entity/entity.api.php";

Sorry, I forgot to add the comment's number at the end of my patch's name.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Hi again!

Sending a new patch, with needed fixes for PHPStan and PHPCS.