No term language awareness inside TermGlossaryManager

I added a MR. Please review it and if there is feedback, I will add it.

Looks great to me, thanks!

I am wondering about the accessCheck(TRUE) here.

I guess the cache should also take the user permissions into account then. Otherwise we will get some inconsistencies.

What do you think?

You are right, this makes sense. I think the "accessCheck()" bit is a security feature since unpublished terms might leak to users that would otherwise not have access to it.

This only leaves the cache clearing, since this cannot be done in a proper manner when we add the roles to it. I would go for a tag based clearing then.

Yep using the user:roles context seems like a good solution.

As for the cache clearing we could use the generic taxonomy_term_list:BUNDLE.

What do you think?

I added a basic handling of the cache invalidation as well as the role information.

Looks like you can use the cache_contexts_manager to generate your cache key suffix:

$contexts = ['user.roles'];
$cid_suffix = \Drupal::service('cache_contexts_manager')->convertTokensToKeys($contexts);

Seems a bit cleaner to me.

I re-did the cache key generation based on your feedback

Wouldn't it be better to use taxonomy_term_list:<VOCABULARY_ID> as the cache tag?

Wouldn't it allow automatic refresh of the cache as soon as a vocabulary term is updated?

Now I understand what you meant with the taxonomy_term_list. I think all your comments are now addressed.

A few phpcs warnings need to be fixed, and then we'll be good for merge.

Thanks for the great work @drcolossos!

Took a few shots, but everything is green now.

BTW: also note that I renamed the service for the "term_glossary_per_node" module to allow inheritance of the arguments. I'm not sure where the term_glossary_per_node service is needed since I would assume that if a service is available more than once, it's behavior is unclear to me.

Ah, I missed that, thanks for warning me about it.

The service shouldn't be renamed as, when the module is activated, it replaces the original service by it's own customized version.

Do you think there is a better way to do that?

I would create a service provider, that replaces the definition with the "term_glossary_per_node". I don't have this module running but this should work as expected.

Hi @drcolossos, made a few tweaks to the MR, could you have a look at it before merging?

Had to update some handler method and hook signatures to properly handle the new langcode parameter used in the updateTermList method.

// Let handler customize default term data array.
$this->handler->buildTermData($term_data, $term, $langcode);
// Allow other modules to modify the term data array.
$this->moduleHandler->alter( 'term_glossary_term_data', $term_data, $term, $langcode);

Impact will probably be limited as this module is still not widely used.

The changes look good. Of course the ServiceProvider needs to be in src/. Good catch!

I would change the signature of hook_term_glossary_term_data_alter to include the $language default of LanguageInterface::LANGCODE_NOT_SPECIFIED. This way, there is "less" breaking of existing code.

@drcolossos is this what you are thinking about?

function hook_term_glossary_term_data_alter(&$term_data, $term, $langcode = LanguageInterface::LANGCODE_NOT_SPECIFIED) {
  // Here alter the term data array.
}

Yes, this is what I meant. The JetBtrains IDEs generate the method signatures for you when using the autocomplete feature, so it might be useful to have. Additionally, it shows the "default" for this parameter.

Ok, added to MR.

Do you think it's ready for merge?

Now that I think about it: how is this handled when the content has no language but the terms have a language or vice versa? Should there be a fallback? Should this be configurable?

Currently, the language of the $parent entity is used to determine the language of the terms.

I guess this code is taking care of it:

$term = $this->entityRepository->getTranslationFromContext($term, $langcode);

Here is what is said in the PHPDoc:

This will check whether a translation for the desired language is available and if not, it will fall back to the most appropriate translation based on the provided context.

Good to know. I use this method quite a lot in our day-to-day work, so I would have assumed something well thought-out ;). Just forgot about this detail. Then I'm all good with the merge!

Merged.

Thank you!

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