Problem/Motivation

If there is an index for e.g. the node entity type and it is using a field provided by a hook_entity_base_field_info() implementation in e.g. module example, then that module will not be added as a dependency on the index config.

Proposed resolution

If the provider of a field isn't the provider of the entity type then add it as dependency on the index config,

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#5 2987156-5--field_provider_dependency.patch1.35 KBdrunken monkey
#5 2987156-5--field_provider_dependency--interdiff.txt1.03 KBdrunken monkey
#3 2987156-3--field_provider_dependency.patch1.21 KBdrunken monkey
#3 2987156-3--field_provider_dependency--interdiff.txt981 bytesdrunken monkey
#2 2987156-2.patch1.29 KBhchonov

Comments

hchonov created an issue. See original summary.

hchonov’s picture

hchonov
Primary language Bulgarian
Location Burgas
commented
Status: Needs work » Needs review
StatusFileSize
new 2987156-2.patch1.29 KB

This patch correctly adds the dependency on the module. Should we do anything about this in \Drupal\search_api\Entity\Index::onDependencyRemoval()?

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Component: General code » Plugins
StatusFileSize
new 2987156-3--field_provider_dependency--interdiff.txt981 bytes
new 2987156-3--field_provider_dependency.patch1.21 KB

Oh, yeah, makes sense! Thanks!

Pretty sure the $key === 'imported' part is site-specific, though, right? Removed in the attached revision.
Looks good, otherwise, though. We'll end up with a lot of dependencies on the module the entity type is provided by, though – maybe we should also check for that? (If the datasource is removed, all its fields should be removed automatically as well. (I hope.))

Regarding onDependencyRemoval(): We just remove fields whose dependencies aren't met anymore, so no need for any changes there, I think.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

Yes.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 2987156-5--field_provider_dependency--interdiff.txt1.03 KB
new 2987156-5--field_provider_dependency.patch1.35 KB

Thanks for reviewing!
However, I think we should check whether this is actually a different module after all. It doesn’t really matter functionally, but I wouldn’t want to see the config export bloated unnecessarily like that.

Also, would still be great to hear from hchonov whether this actually fixes his issue. (Especially with the non-trivial change now.)

hchonov’s picture

hchonov
Primary language Bulgarian
Location Burgas
commented

Oh, I guess I've uploaded the wrong patch, and therefore there is the "imported" field :).

I am sorry about the confusion.

For consistency however I would add the provider always, not only when it is differs from the entity type provider.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented

For consistency however I would add the provider always, not only when it is differs from the entity type provider.

Hm, OK … What’s your opinion on this, Joris?

This discussion aside, does the latest patch work for you?

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

I think more information is better, so I'm inclined to agree with @hchonov here.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs review » Fixed

Democracy is a bitch.
But sure, then let’s stick with #3. Committed.
Thanks again!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.