🇧🇪Belgium @svendecabooter

svendecabooter → created an issue.

Thanks for the MR update. Looks good to me.
I will update the logic in https://www.drupal.org/project/advancedqueue_mail → , so it can keep working with the newly proposed event names.

svendecabooter → created an issue.

svendecabooter → created an issue.

svendecabooter → made their first commit to this issue’s fork.

Thank you!

svendecabooter → created an issue.

svendecabooter → created an issue.

Fixed by https://www.drupal.org/project/ai_usage_limits → module.

https://www.drupal.org/project/ai_usage_limits → is a module that provides usage limit control, although it is limiting the usage per AI provider, not on a per-user or per-role basis.
That might be a good idea for an additional feature in the module (or a submodule).

Could the https://www.drupal.org/project/ai_usage_limits → module help in any way with the usage costs parts, or vice versa?
It does provide the option to calculate the current token cost per provider, and enforce a limit based on that.

svendecabooter → created an issue.

Can you tag a new release, so the D11 compatibility will be available?

Looks good to me.
Just marking as NW for some clarification about the user 1 logic.
I would prefer this to be handled in a separate issue in the issue queue, if possible.

This works in the latest version of ai (1.2.x branch) + ai_provider_openai (1.2.x branch).

Would also like some clarification on this.

svendecabooter → created an issue.

Shouldn't the FieldTextExtractor plugin for Address fields be added to the Address module itself?
Currently we only provide FieldTextExtractor plugins for core field types.
Other modules, like custom_field, provide their own FieldTextExtractor plugin, so presumably this could also happen for Address?

Is this bug still present in the 1.2.x branch?

Those seem like some valid points and good improvements to the AI translation system to me.
To implement some of those, some major refactoring might be needed to the AI Translate system, so not sure if they would all be easily doable.

Currently the field data gets extracted from the entity, based on the field type of each field.
The 3rd party settings available on a field could be leveraged to configure this some more.
E.g. the ReferenceFieldExtractor plugin already does this, to decide whether to translate a certain entity_reference field or not.
Extending this with the option to "optionally disable translation per field" seems fairly doable.

An optional prompt per field is something that would need to be passed along in the metadata of the extracted field values, towards the TextTranslator service. This should also be possible, but would complicate the already somewhat complex array structure of this even more.

Switching providers per field would work in the same way. Actually the \Drupal\ai_translate\TextTranslator::translateContent() method already has a context array parameter implying in the documentation that should be possible, but the implementation code doesn't do anything with it.

We should probably provide a decent framework where configurable context can get passed along during the translation pipeline (text extraction, AI service calling, translation saving), and more events get triggered, so other contrib modules could plugin into that and provide extra tools - if not provided with the ai_translate module itself.

I think this request should probably need some more analysis, and be split out into separate subtasks.

anmolgoyal74: can you add this to the MR?

The change in modules/ai_translate/src/Plugin/FieldTextExtractor/ReferenceFieldExtractor.php did fix the issue I encountered on a website, where untranslated fields (eg regular select list) where left blank in the AI translation.

Tried a few prompts and I did not get any more fatal errors.

Thanks, this fixes the issue when I reproduced this behavior.

svendecabooter → created an issue.

svendecabooter → created an issue.

So for example:

\Drupal\ai\AiFieldTextExtractor\TextFieldExtractor:

#[AiFieldTextExtractor(
  id: "text",
  label: new TranslatableMarkup('Text'),
  field_types: [
    'title',
    'text',
    'text_long',
    'string',
    'string_long',
  ],
)]
class TextFieldExtractor {

  /**
   * {@inheritdoc}
   */
  public function getColumns(): array {
    return ['value'];
  }

  /**
   * {@inheritdoc}
   */
  public function extract(ContentEntityInterface $entity, string $fieldName): array {
    // IMPLEMENTATION.
  }

  /**
   * {@inheritdoc}
   */
  public function shouldExtract(ContentEntityInterface $entity, FieldConfigInterface $fieldDefinition): bool {
    return TRUE;
  }

}

(some of these methods would be in a base class obviously, but for illustration purposes I added them here)

and

\Drupal\ai_translate\FieldTextExtractor\TextFieldExtractor:

#[FieldTextExtractor(
  id: "text",
  label: new TranslatableMarkup('Text'),
  field_types: [
    'title',
    'text',
    'text_long',
    'string',
    'string_long',
  ],
)]
class TextFieldExtractor extends \Drupal\ai\AiFieldTextExtractor\TextFieldExtractor: {

  /**
   * {@inheritdoc}
   */
  public function setValue(ContentEntityInterface $entity, string $fieldName, array $textMeta) : void {
    // IMPLEMENTATION.
  }

  /**
   * {@inheritdoc}
   */
  public function shouldExtract(ContentEntityInterface $entity, FieldConfigInterface $fieldDefinition): bool {
    return $fieldDefinition->isTranslatable();
  }

}

I probably was a bit too enthusiastic with my MR.

I identified multiple problems with just moving the FieldTextExtractor plugins from ai_translate to ai core:

1. The logic in shouldExtract() for each plugin is tightly coupled to translation logic. In it's easiest form, it checks if a field is translatable. In more complex cases, e.g. for Entity References, it checks configuration settings to determine whether to extract a given entity reference.
2. The logic in setValue() is also tightly coupled to translation logic. It assumes the plugin that extracted the field value, will store it again on the exact field same location (but on the translated version of an entity)

The scenario's where text extraction could be useful outside of translation logic, seem to be:

• For ai_content_suggestion: this just needs the extraction part, not the logic for setting the value again on the same field!
• For ai_automators: might be useful to provide the extracted values as tokens to automators? There is already something in place like that, but using the extractor plugins might extend that logic? Haven't looked to deeply into this. But this would also only need the extraction logic I assume?
• ... other places?

Refactoring the FieldTextExtractor plugins to be more generic, while keeping backwards compatibility, seems rather challenging.
I was thinking we could set it up as follows:

New plugin type: AiFieldTextExtractor

• Resides in the Drupal\ai namespace
• Provides implementations for various field types, same as current ai_translate FieldTextExtractor plugins
• Only implements the extract() method + shouldExtract() method / no value setting logic
• Does it need to be limited to extracting text? If not, name it AiFieldExtractor? Haven't thought this through yet :)

Existing plugin type: FieldTextExtractor

• Resides in the Drupal\ai_translate namespace
• Already has implementations for various field types: they will be refactored to extend their AiFieldTextExtractor equivalent
• Only provides the setValue() method.
• extract() and shouldExtract() could be overridden to provide translation-specific logic that is not in the parent AiFieldTextExtractor plugin counterpart
• Could potentially be deprecated in 2.0.0 in favor of a better named AiTranslateFieldTextExtractor plugin / namespace, but that might be nitpicking on my part...

ai_translate and contrib modules extending this module would keep on using the FieldTextExtractor plugins.
Other AI modules could use just the extract logic from the AiFieldTextExtractor plugins, and run with it.

Does that make sense, or is it overly complex and unnecessary? Thanks for giving it some thought.

TODO:

- \Drupal\ai\TextExtractor::shouldExtract() still contains translation-specific logic. This should be moved to the module making use of the Text Extractor service, in this case ai_translate. Not sure how this should be set up.

Added a MR to refactor this.
Marked the existing ai_translate classes / services deprecated, so contrib modules extending them, have time to update their dependencies.

svendecabooter → made their first commit to this issue’s fork.

Setting back to 1.2.x as the correct branch to target.

I tried reproducing this, as I had earlier experienced weird behavior when I had connection problems to my AI API endpoint.
However, in my testing, I get into \Drupal\ai_translate\Plugin\AiProvider\ChatTranslationProvider::translateText(), and into the GuzzleException catching. This then logs the error, but proceeds to return an empty translated text output.

This finally results into database errors trying to save the translation, and the display of the message "There was some issue with content translation.".

So the scenario above might be a separate use case that could be taken into account for improving the error message output.
The error message should be more clear, and a translation should not be attempted to be created I guess?

Tested this MR.

I removed all content_translation permissions from a given user.
They could no longer see the "Translations" tab in that case.

However, when given the "Create AI translation" permission, they could still access the translation URL directly, and a translation was added.
Not sure if this is desired behaviour, or if it should be an AND operation: only create AI content translations when you have "create ai content translation" permission + the appropriate content_translation permission as well.

So the original issue raised in this topic was not yet fixed, AFAIK.

The improved logic to hide the AI Translation column when not relevant, seems to work and is definitely an improvement.

I also took the opportunity to add extra config validation to the schema.
This is useful for when ai_translate settings get set via another means than the settings form, e.g. via a recipe.

• Fixed the entity_reference_depth that was wrongly positioned in the default config
• Added missing language_settings default value
• Updated default reference_defaults values. Now it defaults to empty, since it was referencing entity types that might not even exist on a given site, making the configuration valid more of a challenge.

The last change might be debatable...

svendecabooter → created an issue.

Deprecation warnings have been fixed in the new OpenAiBasedProviderClientBase class.
Pipeline still gives a PHPUnit error in Drupal\Tests\ai\Kernel\Utility\TextChunkerTest
Not sure how this is related to this issue...

svendecabooter → created an issue.

Added a fix that should address this issue.

Thanks, that seems like a good addition.

Could we also take the opportunity to set autowire: true in the services defined in externalauth.services.yml?
Autowiring is supported since 9.3, and since our min. required version is 9.5, that should work.

svendecabooter → made their first commit to this issue’s fork.

It is made backwards compatible from Drupal 10.1 on, with the #[LegacyHook] attribute.
See https://www.drupal.org/node/3442349#hook_convert →
That's already added in this MR - so could still go into 3.2.x if desired.

No longer needed - see 📌 Deprecate module for ai 1.2.x Active

No longer needed - see 📌 Deprecate module for ai 1.2.x Active

Rebasing / fixing of merge conflict causes a new issue in test Drupal\Tests\openid_connect\Functional\Update\IssAllowedSchemaUpdate30004Test::testUpdateHook30003

Not sure how this is related, except that earlier hook also updated plugin config. Maybe it now fails because in our update hook we also update the plugin config further.
Any insights from maintainers as to how to solve this would be appreciated.

Updated the MR to fix a merge conflict with hook_update_x numbering.

svendecabooter → created an issue.

Does this issue still exist on the latest version of the module?

That seems more like an issue of a bad workflow regarding config management...
But the proposed fix is fairly straight forward, so will add it as an extra safeguard.

Improved the logic from ✨ Add permission for resending emails Needs review to make it compatible with other modules, that provide a workaround for the broad 'administer users' permission. Now you can access the feature when you have 'administer users' permission, or the module-specific permission.

svendecabooter → made their first commit to this issue’s fork.

Added the new permission, while keeping the existing access logic intact.
You can now use this functionality if you are able to update user entities (current access logic), OR apply the action to user entities if you have the newly added permission granted.

I guess you would need to debug this via XDebug or something...
Alternatively, you could switch to using the Drupal Symfony Mailer Plus → module with an SMTP transport configured, and install Drupal Symfony Mailer Log → module. This will create a log entry for each outgoing email, making it easier to debug if the mail is going out or not.

Sorry for not coming back to this.
That seems like a useful feature to be added to an optional submodule, for users seeking that functionality.
Or within the module itself as a configuration option (but disabled by default).

Looks good, thanks!

Weird, I can't seem to reproduce this (although I'm testing with Drupal 11.2 currently).
Version 1.3.6 didn't introduce anything that should break this again, so it's probably due to core changes.

@nidhi27 yeah that was my rough idea... not sure if that's easily doable.

I tested this logic, both updating fields created in the 3.x version, and adding new fields.
I have found no real issues with it.
Seems like a good improvement to me.

svendecabooter → created an issue.

Merging now.

OK those seem like 2 valid points to stick with the current approach.
If the current logic would pose unexpected problems, we can still change the implementation later on.

Thanks for the clarification.
Sounds like a good improvement to me.

Isn't this (partially) implemented in 📌 Add token usage to streamed chat Active , or am I reading this incorrectly?

Thanks for the MR. Logic looks good to me.
Only thing I'm wondering: couldn't we use a QueueWorker plugin to process the deletion of the stale log entries?
Then we wouldn't need the batch processing logic within the cron hook.
The processing time could be configured in the QueueWorker plugin - e.g. \Drupal\locale\Plugin\QueueWorker\LocaleTranslation has this set to 30 seconds.

Yeah I have set it up with "Details" wrapper currently.
Would be good to allow for 1-click collapsing / expanding though, to easily switch between "editing" mode and "reordering" mode.

svendecabooter → created an issue.

svendecabooter → created an issue.

Tested patch as well, and works like a charm.