Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Solr filter for entity relationship that has term. #913

Draft
wants to merge 6 commits into
base: 2.x
Choose a base branch
from

Conversation

rosiel
Copy link
Member

@rosiel rosiel commented Nov 30, 2022

GitHub Issue: n/a

Other Relevant Links
Discussion in yesterday's Open Meeting with Simon: How can we make a Facet that lets you filter by Collection? Right now, all sorts of other compound/Book objects are coming up because they use the same Member Of field.

What does this Pull Request do?

Makes new Fields available to Search API that let you filter entity reference fields, and only index the target if the target has a given taxonomy term.

What's new?

  • New Search API Processor (needs to be enabled on the Processors page)

  • New Fields (mirror existing fields, but are "filtered by term")

  • Does this change add any new dependencies? no

  • Does this change require any other modifications to be made to the repository
    (i.e. Regeneration activity, etc.)? no

  • Could this change impact execution of existing code? no

How should this be tested?

  • add some content using the member_of field. Have some be member of Collections, and others member of Paged Content.
  • install this PR and clear cache.
  • Go to /admin/config/search/search-api/index/default_solr_index/processors and enable "Entity Reference, where the target has a given taxonomy term". Save the page.
  • Under "Fields", see a new field in the Content section: "Member of (by term URI) [islandora_object] (entity_reference_with_term__node__islandora_object__field_member_of)"
  • Select it, configure it to only show (books, collections, whatever). Make sure if you're using Collection that you select the same term you applied to your content. You can select multiple terms and say if you want all or any.
  • Reindex your content
  • Go to Facets, and create a facet for this field you added. (give it a useful name)
  • Go to Blocks, and place the block you just created. (find it with the name you gave it above)
  • Do a search, and see only the desired "types" in your facet.

Documentation Status

  • Does this change existing behaviour that's currently documented? no
  • Does this change require new pages or sections of documentation? maybe.
  • Who does this need to be documented for? site builder
  • Associated documentation pull request(s): ___ or documentation issue ___

Additional Notes:

Interested parties

Tag (@ mention) interested parties or, if unsure, @Islandora/committers

src/Plugin/search_api/processor/EntityReferenceWithUri.php Outdated Show resolved Hide resolved
src/Plugin/search_api/processor/EntityReferenceWithUri.php Outdated Show resolved Hide resolved
$require_all = $search_api_field->getConfiguration()['require_all'];
$field_name = substr($search_api_field->getPropertyPath(), 28);
$field_name = str_replace($type_and_bundle_prefix, '', $field_name);
if ($content_entity->hasField($field_name)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably check !isEmpty here as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now do a

      if (!$content_entity->hasField($base_field)) {
        return;
      }

      // Consider indexing each entity referenced through this field.
      foreach ($content_entity->$base_field->referencedEntities() as $target_entity) { ... }

I think calling ->referencedEntities() will fail very gracefully if it's empty.

if ($require_all) {
if (count(array_intersect($referenced_tids, $filter_tids)) == count($filter_tids)) {
// "All" and all the terms specified are present.
$label = $target_entity->label();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks to be outside of the foreach where it's instantiated and could be possibly be undefined?

@rosiel
Copy link
Member Author

rosiel commented Dec 12, 2022

Thank you for the comments! I'm working on addresing them. I realized I also need to convert it to store the URI, à la the Node has Term Condition, if I want to use it with the Starter Site. And the new Mac OS, Ventura, may be interfering with my debugging on port 9003? I don't have time to solve this just this minute, but will get to it soon.

/**
* {@inheritdoc}
*/
public static function create(ContainerInterface $container, array $configuration) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When enabling the processor I get:

Warning: Declaration of Drupal\islandora\Plugin\search_api\processor\Property\EntityReferenceWithUriProperty::create(Symfony\Component\DependencyInjection\ContainerInterface $container, array $configuration) should be compatible with Drupal\Core\TypedData\DataDefinition::create($type) in include() (line 18 of modules/contrib/islandora/src/Plugin/search_api/processor/Property/EntityReferenceWithUriProperty.php).

include('/opt/www/drupal/modules/contrib/islandora/src/Plugin/search_api/processor/Property/EntityReferenceWithUriProperty.php') (Line: 578)
Composer\Autoload\ClassLoader::Composer\Autoload\{closure}('/opt/www/drupal/modules/contrib/islandora/src/Plugin/search_api/processor/Property/EntityReferenceWithUriProperty.php') (Line: 432)

search_api's properties are intended to be a data definition and appear to not be architected to directly support DI. Normally when wanting to inject you would need to implement the ContainerFactoryPluginInterface which is not possible here.

Is a few options which could work around this such as implementing your own static factory method to instantiate the property or re-working the plugin itself to pass in the requested values directly as opposed to the property itself. Alternative could be just making static functions in the property class as opposed to injecting (@adam-vessey clarify if I'm off here?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we discussed how ContainerFactoryPluginInterface doesn't work here because the Property is not a Plugin, when I was doing this.

So without DI, then we have these ugly things:

$entity_type_manager = \Drupal::entityTypeManager()->getStorage('field_config')

and

$utils = \Drupal::service('islandora.utils');

Is this what you were referring to with "just making static functions in the property class as opposed to injecting"? I'm fine with that.

"Implementing my own static factory method" sounds hard. :(

"plugin itself to pass in the requested values directly as opposed to the property itself" I'm not sure I follow. Right now the plugin does

$properties[$fieldname] = new EntityReferenceWithUriProperty($definition, $this->entityTypeManager, $this->utils);

and the Property's __construct() function accepts that signature... OOOOHH you're saying we could maybe remove the Property's __create() function?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implementing [a] static factory method

Really isn't that difficult. As in, it's effectively the same as ContainerFactoryPluginInterface, just without being passed the container... so similarly have a static method like the ::create() it described, and then use the \Drupal global class dealio to access things from the container. So, something like:

class EntityReferenceWithUriProperty [extends whatever class and implements whatever interfaces] {
  [...]
  public static function createInstance(array $definition) {
    return new static(
      $definition,
      // \Drupal::entityTypeManager() could probably equally work, anyway.
      \Drupal::service('entity_type.manager'),
      \Drupal::service('islandora.utils')
    );
  }
}

Sure, it might does still do a bit of binding with Drupal proper; however, the bigger issue is allowing the services to be passed in the constructor that this would address (facilitating testability, should it be pursued), but doing so without requiring invoking classes to know what needs to be passed to the constructor other than what they need to know (like the $definition, in this case).

Then, instead of the new to create, you can just hit this new static factory method, with something like:

$properties[$fieldname] = EntityReferenceWithUriProperty::createInstance($definition);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On another note, looks like there should be schema definitions for any properties...

An example of those in search_api proper: https://git.drupalcode.org/project/search_api/-/blob/8.x-1.x/config/schema/search_api.processor.schema.yml#L220-271

Where they seem to be targeted, from the index config entity: https://git.drupalcode.org/project/search_api/-/blob/8.x-1.x/config/schema/search_api.index.schema.yml#L40-41

Might get a little messy given what I think would correspond to the key there, dynamic property names as per $fieldname over at https://github.com/Islandora/islandora/pull/913/files#diff-a6e10f5851d0df552f0c1523868a930cc3aac263d2c54582694ff5f979977bccR123-R124 ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another potential alternative to deal with DI things could be to push off the building out of the lists of things another level, to define FormElements that are aware of what they can/should display (under the assumption that FormElement plugins can work with the ContainerFactoryPluginInterface themselves), then the "form building" in the config form shouldn't need to have things injected at all?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants