feat(data-producer): Introducing a taxonomy term autocomplete data producer. #1105
Conversation
klausi
left a comment
klausi
left a comment
There was a problem hiding this comment.
Cool, I have a couple of questions, not sure how we should handle them.
| 'field' => $field, | ||
| 'match_string' => $match_string, | ||
| 'prioritize_start_with' => $prioritize_start_with, | ||
| 'limit' => $limit, |
There was a problem hiding this comment.
vocabularies might also be interesting for consumers of this hook?
There was a problem hiding this comment.
well, not for this use case, the input here is a field of entity type of a bundle, the vocabularies are defined directly on the field, so additional vocabularies input makes no sense here. maybe we should rename it to TermFieldAutocomplete?
| * id = "term_autocomplete", | ||
| * name = @Translation("Term autocomplete"), | ||
| * description = @Translation("Returns autocomplete items matched against | ||
| * given query in given vocabulary"), produces = @ContextDefinition("list", |
| // Process the terms returned by term autocomplete query. | ||
| $terms = []; | ||
| foreach ($query->execute() as $match) { | ||
| if ($term = $this->getTermStorage()->load($match->tid)) { |
There was a problem hiding this comment.
this is expensive to execute a load for each loop iteration.
Could we load all terms a once?
Why do we need full term objects? It would be enough to return term ID + name, or what else do we need for an autocomplete?
We also don't use the entity buffer here like TaxonomyLoadTree does, is that intentional?
There was a problem hiding this comment.
two reasons for loading entities:
- this is how every entity autocomplete works in drupal, it loads entities
- we need to resolve custom synonyms field. anybody may need to resolve other custom taxonomy term fields
this shouldn't matter much because the results are cached. entities are also cached, but we can use buffer, true
There was a problem hiding this comment.
I think it will be better to return ids only. This can be then chained into entity_load_multiple data producer (via compose), if anybody needs entities. Should make everybody happy and will simplify everything.
| FieldContext $context | ||
| ): ?array { | ||
| if ($limit <= 0) { | ||
| $limit = 10; |
There was a problem hiding this comment.
should we define an upper limit here on the query so that this cannot be abused for DoS attacks?
What would be a good max value? 100? 200?
We have a similar case in TaxonomyLoadTree where MAX_DEPTH is not enforced correctly.
| // Base of the query selecting term names and synonyms. | ||
| $query = $this->database->select('taxonomy_term_field_data', 't'); | ||
| $query->fields('t', ['tid']); | ||
| $query->condition('t.vid', $vocabularies, 'IN'); |
There was a problem hiding this comment.
we need to add the taxonomy_term_access tag here, right?
and how does that affect caching? Does that mean we cannot cache the result of this resolver?
There was a problem hiding this comment.
every query is cached based on query and input variables out of the box.
we additionally tag the result by returned term ids and taxonomy_term_list
There was a problem hiding this comment.
I see, so the current user with their access rights would be an input for this query to consider for caching?
How does drupal core implement taxonomy autocompletes and caching?
|
@klausi can you recheck?
|
klausi
left a comment
klausi
left a comment
There was a problem hiding this comment.
Ok, then I think we can use this as a first step.
The Drupal core autocomplete in EntityAutocompleteMatcher and TermSelection with entity queries, but there you cannot sort, so does not work for our use case. I also did not see how they handle caching there. Since we need to compose this with an entity load data producer anyway and access is handled there as well I think this is fine.
|
Test coverage is missing, not a big risk for this standalone data producer. I'm thinking how we should deal with this in the future, ideally we should have a basic test for every new data producer we create. |
will try to make a test for it in a follow up, because in original task there was no time reserved for the test |
2 participants
Data producer which allows to perform autocomplete queries on given field of given entity type of given bundle.
Features: