🇧🇪Belgium @svendecabooter

svendecabooter → created an issue.

Linking the issue in the custom_field issue queue, where this functionality could be added

This seems to fix D11 compatibility.
Can this be merged and a new release tagged?

+1 - this module is blocking us from upgrading a site to D11.

Can this be merged, and a new release tagged?

The dependency change is probably not needed.

I thought it would remove the ai_prompt entity upon uninstalling the ai_translate module, but after some extra testing, it seems it does not.
It just lets the entity exist. But the prompt type does get removed, so a stale ai_prompt entity remains, without a valid prompt type.
Not sure if there is a way to get that entity removed as well (if that is desired).
Maybe general logic for deleting ai_prompt entities when their dependencies are removed, is something for a separate issue?

Created a new branch with fixes I noticed were missing in the merged functionality.
Could you review and add as well?

Test seems to fail - still needs to be addressed.

svendecabooter → changed the visibility of the branch 3.x to hidden.

svendecabooter → changed the visibility of the branch 3010914-product-variation-access to hidden.

svendecabooter → created an issue.

@valthebald

This logic was taken from the documentation in the AI module.
Seemed logical to me, but indeed the update hook would just need to focus on the prompt that is currently active on that particular website, not what the module ships with by default...

svendecabooter → created an issue.

svendecabooter → created an issue.

Hi richgerdes,

Thanks for the feedback. Moving this back to the original queue.

I haven't been active in the queue indeed. We're using the module on a few sites, and it mainly works as intended, so no direct need.
The most urgent need is a D11 release indeed, so I'll check if I can get that out of the door.

I'll then look at the roadmap to see if I can assist with moving some of these things along.

If there is no response from the project maintainers, I'd be willing to volunteer as co-maintainer as well, to move this module forward.
I can opt projects into security advisory coverage.
I could then look into working together with sokru where relevant or appropriate.
Let's see if there is any response here.

Moving to the Drupal.org project ownership queue, as per instructions in https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or... → , since there has been no reply for 14 days.

Yes, since 1.2.x requires ^10.4 || ^11, I figured it was safe to use those.

See draft MR for a proof of concept of how this could work.

This would validate different prompt types differently.
See screenshots from output of the Config Inspector → module:

ai.ai_prompt.suggest_tags__suggest_tags_default
Uses general ai.ai_prompt_text.* schema

ai.ai_prompt.ai_translate__blaat
Uses specific ai.ai_prompt_text.ai_translate schema

svendecabooter → created an issue.

svendecabooter → created an issue.

svendecabooter → created an issue.

The reason why the '#parents' property might need to be added, might need to be clarified, since I don't know exactly.
Is it because the form element is rendered within a tree structure, that it might be needed?
Or because there might be multiple ai_prompt form elements on the same form page?

I had a scenario where both of those were true, and adding it made everything work for me, but I don't know which of the scenario's it fixed exactly :)

svendecabooter → created an issue.

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

Wouldn't it be a better DX if there were a dedicated config action in the Key module?

Then developers would more easily find out how to do so.

Updated the user stories in the original description, for extra clarification.

All dependencies of this module now have a release, so I tagged an alpha2 version.
This should allow the module to work without patches.

Thanks for considering linking to it from the project page.

Thanks for the heads up.
It seems to work fine on my clean install, but perhaps I have another library activated that uses the once library on the page.
Closing this issue again, in favor of the followup issue 🐛 Add drupal/once library dependency Fixed

svendecabooter → created an issue.

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

Thanks for the MR! Merged now.

svendecabooter → created an issue.

svendecabooter → created an issue.

svendecabooter → created an issue.

Thanks, fix merged!

Thanks, fixed now.

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

I think OR logic should be fine...
You are giving explicit "Create AI translation" permission to a given role, so it's not unreasonable you would expect them to be able to create AI translations then, even if they can't create manual content translations...

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.