Consider field storage dependency removal on Index

+++ b/src/Entity/Index.php
@@ -1403,6 +1417,12 @@ class Index extends ConfigEntityBase implements IndexInterface {
+      if ($entity instanceof FieldStorageConfig) {
+        // @todo Make this generic through the datasource.
+        // Remove the deleted field.
+        $field_id = 'entity:' . $entity->getTargetEntityTypeId() . '/' . $entity->getName();
+        $changed = $this->removeField($field_id);
+      }

So the hard part is being able to route this through the data source so that it can give us the field_id to remove.

Maybe something like $datasource->getFieldIdsForDependencies($dependencies) or something like that?

Thanks a lot for reporting this problem and providing a patch! It already looks quite good. A few notes, though:

1. +++ b/src/Entity/Index.php
   @@ -1403,6 +1417,12 @@ class Index extends ConfigEntityBase implements IndexInterface {
   +        $changed = $this->removeField($field_id);
   

   That needs to be a |= instead of an =, unless I'm mistaken. Otherwise, other changes might be ignored.

2. +++ b/src/IndexInterface.php
   @@ -558,4 +558,15 @@ interface IndexInterface extends ConfigEntityInterface {
   +  public function removeField($field_id);
   

   That's a bit weird to have if there's no addField() method.
   There is already the method of $index->getFields()[$field_id]->setIndexed(FALSE, TRUE) which is a bit more verbose, but should work fine.

- Making the field name generic through datasource

How about just making getFieldDependencies() public and letting it return a mapping between Search API fields and dependencies (no matter which way – we just have to keep in mind that that's an N:M relationship). It would then be called from the index automatically, of course, the datasource would just return (in the case of the ContentEntity datasource) the provider of the entity type it's configured for as the sole dependency. And the onDependencyRemoval() code would have it easy to determine which fields need to be removed. (So it seems for both (existing) use cases the format [$dependency => $field_ids] would be handier.)

- Removing additional fields

I think that would better be handled along with the previous item – when getting the dependencies from the datasource, it would just list all Search API fields that depend on each of the Field API fields – i.e., we would know that both entity:node/field_editor and entity:node/field_editor:entity:name depend on the field_editor field. (However, we of course have to keep in mind when defining the return value of getFieldDependencies() (and implementing it) that, e.g., entity:node/field_editor:entity:field_project would depend on two fields – field_editor and field_project.

The point that we should depend on field storages, not field instances, was already discussed in #2472917: Make index configuration depend on field storage not instances. Since there's no patch for that issue, yet, it's OK to fix it here, of course. I've just closed that issue as a duplicate now.

Anyways, thanks again for the great work!

Thanks for the reworked patch! Looks already better, but I don't think you've correctly understood my proposal?
It was about re-using the already existing (protected) getFieldDependencies() by making it public and letting it return a mapping of all dependencies mapped to the fields that depend on them.
If done properly, this would already take care of additional fields, too.

+++ b/src/Entity/Index.php
@@ -1425,6 +1425,14 @@ class Index extends ConfigEntityBase implements IndexInterface {
+        $this->getFields()[$field_id]->setIndexed(FALSE, TRUE);

If, for some reason, the field isn't present in the index, this will lead to a fatal error. Please check for the existence of the field before calling the method (and only set $changed if it is there.

If the caching issues are not related, please create a new issue for them. It seems we just shouldn't cache the unindexed fields?

We discussed that and couldn't really make sense of what you were suggesting since the function seems to be opposite of what we need (needs fields, returns dependencies).

But I think I understand what you are saying now. That it should just return the mapping for all fields and their dependencies, not just those that are passed in?

We can try that tomorrow.

But I think I understand what you are saying now. That it should just return the mapping for all fields and their dependencies, not just those that are passed in?

Exactly, yes. Instead of just returning an array with the dependency keys as values, put the dependency keys into the returned array's keys and store the Search API field(s) each dependency comes from in the values. That way, you can easily use the same method for both determining the dependencies (array_keys()) and determining the fields which should be removed when a certain dependency is removed (foreach ($dependency_fields[$dependency] …). Less code, less potential for bugs. And the issue with additional fields should also already be solved (though it seems we don't handle non-standard entity references, yet (i.e., non-Field API properties that reference entities) – but that's a different issue).

Thanks a lot for your work on this!

First off, some remarks on the code (interdiff) I had at a glance:

1. +++ b/src/Entity/Index.php
   @@ -636,7 +648,7 @@ class Index extends ConfigEntityBase implements IndexInterface {
   +      \Drupal::cache()->set($cid, $this->fields, Cache::PERMANENT, Cache::mergeTags($this->getCacheTags(), ['entity_field_info']));
   

   We currently don't use the short array syntax in this module, I think mixing this is more confusing than useful.

2. +++ b/src/Entity/Index.php
   @@ -1425,6 +1427,38 @@ class Index extends ConfigEntityBase implements IndexInterface {
   +          $fields = array();
   +          // @todo: There is no field that is going to be removed.
   +          foreach ($this->getFieldsByDatasource($datasource_id) as $field) {
   

   What is that @todo supposed to mean?

3. +++ b/src/Entity/Index.php
   @@ -1425,6 +1427,38 @@ class Index extends ConfigEntityBase implements IndexInterface {
   +                    $this->getFields()[$field_name]->setIndexed(FALSE, TRUE);
   

   I think you can just save the fields in a variable before and then call setIndexed() and unset() when removing fields.

4. +++ b/src/Plugin/search_api/datasource/ContentEntity.php
   @@ -580,13 +580,14 @@ class ContentEntity extends DatasourcePluginBase {
   +    // @todo: Consider additional fields when calculating dependencies?
   

   What do you mean with that? Don't you do that already?

5. +++ b/src/Plugin/search_api/datasource/ContentEntity.php
   @@ -580,13 +580,14 @@ class ContentEntity extends DatasourcePluginBase {
   -      $this->addDependencies(array('config' => $field_dependencies));
   +      $this->addDependencies(array('config' => array_keys($field_dependencies)));
   

   As said, now that that method is public I would move this code to the Index class (since it will be the same for all datasources).

6. +++ b/src/Plugin/search_api/datasource/ContentEntity.php
   @@ -625,7 +626,26 @@ class ContentEntity extends DatasourcePluginBase {
   -            $field_dependencies[$field_definition->getFieldStorageDefinition()->getConfigDependencyName()] = TRUE;
   +
   +            // Make a mapping of dependencies and fields that depend on them.
   

   Please remove the empty line.

Then, I'm not sure I understand the purpose of the $useComputedFieldsCache property correctly.
At the very least, if it is FALSE, I don't think you should set the persistent cache to the newly calculated value?

Also, for #4.1, isn't that what the new entity_field_info cache tag solves? Otherwise, why was that added?

And #4.2 should be solved by just clearing our cache when the index is saved. Which, I think, our cache tags should already take care of? Otherwise, we should debug it, but if it hasn't got anything to do with this issue, I wouldn't want to introduce a haphazard fix here, which might (as you say yourself) interfere with our solution for this issue.
(If it makes testing harder/impossible, we'll just have to solve that issue first, before working further on this one.)

Speaking of tests: If possible, it would be great to get some automated test coverage for this issue. It's turning out really complex, and once it's fixed we should ensure that it stays fixed.

Currently, I have a problem on onDependencyRemoval() in Index when calling $this->getFieldsByDatasource($datasource_id). It gives me list of fields on the given datasource but without (Search API) fields that have dependency on the Drupal field that is going to be removed. I guess that's happening because of computeFields() and the $useComputedFieldsCache flag we added, but cannot confirm.

Hm, maybe for this code, with the sensitive time it's executed (in between various config changes) it might just be easier/better to only use low-level operations instead of our helper methods. I.e., loop through $this->options['fields'] directly, and unset the removed fields directly there. Maybe that will help? And it might be possible to merge some of the loops that way, because currently your looping (explicitly and implicitly) a lot through the datasources and fields. Maybe just try to build a more suited data structure for the method and then go with that.

If I can help you with something else, feel free to ping me again via mail or IRC.

On the caching changes, we worked on that together, it is a bit complicated. Field definitions are pretty wrong in HEAD because of that, they're at least one save delayed. Which isn't *directly* related but it makes it very hard to debug/work on this issue.

First, those caches need to be invalidated if new fields are added, which is why we add the entity_field_data cache tag. However, that's only the start.

When you are saving the index and change the fields, we need to request them again during presave. in HEAD, that doesn't work at all because it just keeps using the computed field arrays that haven't been reset. So when the field configuration changes, then we now unset the computed field arrays. But even that is not enough, because it falls back to the cache and loads that again (which has not yet been invalidated as the view has not yet been saved).

That's why we added that flag to prevent it from using the cache at all when it has been changed in the current request. We could also delete the cache, but that method is called for every field so we would repeatedly have to delete that cache.. and there might be parallel requests to the site that are building it again, who knows. But yes, we also shouldn't write the cache if that flag is set.

@ Sascha: Thanks for the explanation! Makes sense, of course.
Still, I a) think the solution looks a bit hacky and b) worry that with just the check in setOption() this will not cover all cases. I.e., there's also setOptions() and set() which might change the fields, and maybe we'll add other code later.

Safer, I think, would be to maybe store alongside the cached data the fields options for which it is valid – i.e., make the cache self-validating. Maybe with just a hash of $this->options['fields'] (and additional fields) attached to the data? That way, you could right away detect when the information that was used to build the cache isn't the one for which you want the fields data now – no matter what events led to this situation. (On the other hand, I don't think this is a pattern employed elsewhere (e.g., in Core), so maybe there's a good reason not to do that. I'ts just an idea.)

Also, if we end up agreeing on using the flag instead, the property should be added to the __sleep() method (I'd say).

It should be set only if it is TRUE?

Yes, I'd certainly say so. Otherwise you'll cache the fields information for a version of the entity which isn't yet saved in the database – if something changes the fields along the way, you'll end up with a bad cache.

Cannot use $this->options['fields'] as it only contains type and not property path what we currently use. Should we modify options, or getFieldDependencies()?

If that's the only issue, the property path can be trivially extracted from the field's machine name, with list(, $property_path) = Utility::splitCombinedId($field_id).

Ok. This works. Removed the flag and instead doing it with that hash.

Also had the problem that it still removed the index despite saying it would not. Which was somehow related to static caches that had already removed the field and didn't know about it anymore. So if I find an unexpected field now, I'm just flagging the entity as changed so that core recalculates the dependencies and then we're fine.

This is great. I discussed this with Berdir in Drupalcamp Vienna and we need one more test that tests the same use case but with the additional fields. Eg, when you add a the title of an image to the search index as well and we remove that field, we need to make sure that also updates the index so that it no longer uses that additional field.

Added tests tag.

Yeah, the dependencies code certainly isn't perfect yet, but this seems like a good start and this improves the situation a lot.

Looks great, rtbc++.

Thanks for implementing it like this, this does look a lot a lot better now.

Still, there were several things suboptimal about this, see the attached patch.

Also a few remarks:

1. +++ b/src/Entity/Index.php
   @@ -323,6 +324,10 @@ class Index extends ConfigEntityBase implements IndexInterface {
   +    if ($name == 'fields') {
   +      // Make sure we recalculate the computed fields.
   +      $this->fields = NULL;
   +    }
   

   +++ b/src/Entity/Index.php
   @@ -331,6 +336,8 @@ class Index extends ConfigEntityBase implements IndexInterface {
   +    // Make sure we recalculate the computed fields.
   +    $this->fields = NULL;
   

   These could also check whether the fields actually changed. But probably unnecessary for now, maybe when we move it to its own property (cf. #2317771: Move processor and/or field settings to top-level properties on the index?).

2. +++ b/src/Entity/Index.php
   @@ -1169,12 +1181,12 @@ class Index extends ConfigEntityBase implements IndexInterface {
   +    $this->applyLockedConfiguration();
   +
        // Stop enabling of indexes when the server is disabled.
        if ($this->status() && !$this->isServerEnabled()) {
          $this->disable();
        }
   -
   -    $this->applyLockedConfiguration();
   

   Why this change, shouldn't that be completely the same? Neither does the locked configuration depend on the status, nor is it possible to lock the index's server.

3. +++ b/src/Entity/Index.php
   @@ -1405,8 +1425,49 @@ class Index extends ConfigEntityBase implements IndexInterface {
   +          else {
   +            // If the field dependency does not exist, it is possible that we
   +            // already updated the configuration to remove that dependency.
   +            // Ensure that we return TRUE so that the new dependencies are
   +            // checked.
   +            $changed = TRUE;
   +          }
   

   Doesn't mean that we'll always return TRUE when a field gets deleted, no matter whether we index it, or whether it's even on the same entity type?
   That sounds pretty inefficient, would be great to find out how we can more reliably detect that case, and/or what causes it.
   Or does returning TRUE not effect a significant performance penalty?

Also, what exactly doesn't work about ("deeply") nested entities? To me, it looks like it would just recurse again. (But the code is pretty complicated, of course, so I can't be sure.

It would also be great not to hard-code :entity: as the separator for an entity reference – even though that's in Core like this, it's still possible to have a reference to an entity in some other way.

But sure, after these questions are cleared up we can at least commit this as a temporary partial fix and try to improve on it subsequently. Would be great if one of you could then create the follow-up issue, to make sure a) we don't forget it and b) it explains correctly what is still missing.

3. No. We're only called if we actually depend on the thing being deleted. There is no other way as far as I see.

3. No. We're only called if we actually depend on the thing being deleted.

Ah, of course, my mistake.

+++ b/src/Plugin/search_api/datasource/ContentEntity.php
@@ -899,11 +897,14 @@ class ContentEntity extends DatasourcePluginBase {
+        // @todo Support deep nesting.

Is there an issue we can link to?

Overall I think the code here looks great, is it possible to add a testcase for this in the dependencyRemovalTest as well? That's a kernel test so probably a different test to the one added in the integration test. But that makes it easier to test the code in isolation.

1. +++ b/src/Datasource/DatasourceInterface.php
   @@ -201,4 +201,17 @@ interface DatasourceInterface extends IndexPluginInterface {
   +   * @return string[][]
   +   *   An associative array with the keys being dependency names of config
   +   *   entities on which the given fields depend. The values are containing the
   +   *   IDs of the fields which depend on the given config entity.
   +   */
   +  public function getFieldDependencies(array $fields);
   

   I wonder if that isn't needlessly restrictive. Why not also allow non-config dependencies there, by using an additional level of nesting?

2. +++ b/src/Entity/Index.php
   @@ -1394,6 +1395,32 @@ class Index extends ConfigEntityBase implements IndexInterface {
      /**
   +   * Retrieves information about the dependencies of the indexed fields.
   +   *
   +   * @return string[][][]
   +   *   An associative array, keyed by datasource IDs of this index and
   +   *   containing the respective field dependency information of that
   +   *   datasource, as described for the return value of
   +   *   \Drupal\search_api\Datasource\DatasourceInterface::getFieldDependencies().
   +   */
   +  public function getFieldDependencies() {
   

   Several things here:
   Why is this public? And, if there is a good reason, the method should definitely also be on the interface. No public methods without interface, as a rule.
   Why does it return the dependencies nested by datasource? Does that really provide any benefit? If there is really a need for that, we could also fulfill it by just adding an $datasource_id = NULL parameter.

3. +++ b/src/Entity/Index.php
   @@ -1450,6 +1477,11 @@ class Index extends ConfigEntityBase implements IndexInterface {
   +    // Calculate field dependencies.
   +    if ($field_dependencies = $this->getFieldDependencies()) {
   +      $dependency_data['config'] = call_user_func_array('array_merge', $field_dependencies);
   +    }
   

   This will have the complete wrong format for $dependency_data. Please add a loop that correctly nests it within 'always' and 'field'.

4. +++ b/src/Entity/Index.php
   @@ -1548,6 +1580,23 @@ class Index extends ConfigEntityBase implements IndexInterface {
   +        // Remove a field from the index that is being removed from the system.
   +        if (isset($dependency_objects[$name]) && $dependency_objects[$name] instanceof FieldStorageConfigInterface) {
   +          if (!empty($dependency_sources)) {
   

   This is a completely different style than the rest of the method. Instead, add your code within the current framework, checking for a $plugin_type of 'fields' and then removing the fields when they come up there.

5. +++ b/src/Entity/Index.php
   @@ -1548,6 +1580,23 @@ class Index extends ConfigEntityBase implements IndexInterface {
   +            $changed = TRUE;
   +            $this->save();
   

   Unless I am very mistaken, the entity should never be saved inside an onDependencyRemoval() method.

See the attached patch for some further corrections.

Furthermore, I think the ContentEntity::getFieldDependenciesForEntityType() method takes a needlessly difficult approach. Instead, it should just take the property paths, descend into them (see \Drupal\search_api\Utility::retrieveNestedProperty() for example code in that direction) and, at each level, check whether the property is an instance of FieldDefinitionInterface – if so, add the necessary dependency. (That way, #2656916: Overhaul or remove ContentEntity::getFieldDependenciesForEntityType() would also be fixed – and adding code for other kinds of dependencies would be much easier.)

+++ b/src/Plugin/search_api/datasource/ContentEntity.php
@@ -899,11 +897,14 @@ class ContentEntity extends DatasourcePluginBase {
+        // @todo Support deep nesting.

Is there an issue we can link to?

More importantly: is this actually correct? As said before, to me it looks like this would recurse correctly also for "deeply" nested fields.
We should definitely add a test for this, either to verify it fails and needs to be fixed or to make sure it works and stays working.
As you say, adding this to the existing DependencyRemovalTest would also make a lot of sense, yes. Although, of course, having both is even better, so we can naturally also keep the addition to the integration test.

Also, the age of the last patch is not an issue, an interdiff could still make sense. But if you mostly just re-rolled, you're right, it would indeed be pointless.

Managed to find the bug, re-wrote the field dependency detection code, but it's still there. The problem is (both with the previous and my current code) that, if we depend on the field storage config (instead of the field config itself), that at the point where onDependencyRemoval() is called the actual field configs have already been deleted – and with them our internal record of having them on the datasource. Thus, at the point where the field is actually deleted (not at the time where the resulting config chances are displayed – that correctly lists the index as "to be updated"), we can't actually find any dependency to the field, thus don't remove any dependency, $changed remains FALSE and the index is deleted (even though it wouldn't even have any (reported) dependency on the deleted entity anymore (the field would remain on the index, though, which is of course not OK).
The underlying problem is that the deletion of the storage upon deletion of the field config is not actually triggered by a dependency, but by a postDelete() operation. (Come to think of it, it's a bit of a mystery why the index is even listed on the field deletion page. Probably special code to include both dependents of the field and the storage (when both would be deleted).)

In any case, I'm attaching my current state of work. Any ideas or input would be much appreciated – not sure how we can solve this properly.

(Come to think of it, it's a bit of a mystery why the
index is even listed on the field deletion page. Probably special code to
include both dependents of the field and the storage (when both would be
deleted).)

Yes. That was a critical that I worked on, in Barcelona IIRC.

Ah, that at least explains that. Thanks!

Any ideas for solving our problem here?

What I thought about that might help is adding caching for our dependency data. That way, the dependency data built for the "Predicted config changes" list would also be used for the actual dependency removal process. The caching would have to be persistent, though, as a cloned version of the index will be used for the "prediction" operation.

The problem here, however: I don't think it's impossible, internally, to remove a field config (or config entity in general) without first doing a "dry run" dependency check. And in that case, this would still fail. So, probably not practical either.

Next idea: Just storing a field's dependencies along with its other information in the index. A bit brute force, maybe, but should generally be fine. Would at least be a solution if we can't think of any cleaner one.

This implements the "saving dependencies with the fields" method.
Seems to work well enough – I guess we should just commit that, we can always come back and improve on it later, if we have an idea.

Anyways, reviews welcome!

Seems #2670372: Index settings do not synchronize across environments fixed that problem. Attaching a re-roll of #45.

Messed up while re-rolling, this one should work.

Still applies – no volunteers for reviewing?
Then unless anyone objects, I'll commit this early next week. High time this finally got fixed.

This is not an easy patch to review ;)

Can't spot anything obviously wrong, seems to work, so +1 from me.

OK, then let's go the alternative route: commit first and then wait for people to complain.
Anyways, thanks for trying it out and looking it over!

Committed.

I looked at this while on the plane earlier today, these were my remarks:

Overall, I think this looks great, I did spot a couple of small suggestions.

1. +++ b/src/Datasource/DatasourceInterface.php
   @@ -195,4 +195,17 @@ public function getEntityTypeId();
   +   *   An array of property paths on this datasource, keyed by field IDs.
   

   /s/IDs/ID/?

2. +++ b/src/Entity/Index.php
   @@ -1121,6 +1123,11 @@ public function preSave(EntityStorageInterface $storage) {
   +    // Since we change dependency-relevant data in this method, we can only call
   +    // the parent method at the end (or we'd need to re-calculate the
   +    // dependencies).
   

   This is a great comment!

3. +++ b/src/Entity/Index.php
   @@ -1503,8 +1536,25 @@ public function onDependencyRemoval(array $dependencies) {
   +                // In case the field is locked, enable it before removing.
   

   This is a great comment, it perfectly describes what's going on. I'd only change "enable" for "unlock".

I think "property paths keyed by field IDs" is fine like it is, but you're right about 3. – "enable" doesn't really make sense there. Committed the follow-up, thanks!