Discuss: Unify translate operations over ai

I think that if this service should exist it has to exist in the ai_translate module, because if we start adding combinations of operation types based on what they solve rather on how they solve it and want to be consistent, there will probably be 100s of combinations in the AI core module, which will be impossible to manage.

Having a dedicated third party module or third party service for that makes more sense in that case.

Adding related issue; depending on where this issue lands, we could leverage ai_translate more.

@valthebald - code review is done from my side, the ai_translate.settings check and the setChatSystemRole method is important. Rest is optional if you think they are improvements, otherwise just skip.

I'll let Frederick do the feature review since he knows the module better, but from what I see in the code, it mostly works the same.

Oh also the PHPCS issues needs to be fixed.

Fixed most of the comments (2 left are for the future).
Also, brought back editing of default translation prompt (patch to have per-language left it hidden) and display of default prompt that has a condition

Thanks @valthebald for this MR!

I pushed and revert a commit in favour of this comments:

1. In ai_translate.install, is ai_translate_requirements() still required? I guess not.
2. In ai_translate.module, _ai_translate_check_default_provider_and_model() seems also outdated to me.
3. In AiTranslateForm.php, line 114, $default_model = $this->providerManager->getSimpleDefaultProviderOptions('chat'); should not use the chat provider type anymore.

I added some feedback as well after testing this in ✨ Fallback to ai_translate configuration Active
There are also some comments from Marcus to be addressed still

In general though it seems like the right direction, and is really helpful to have, thank you for the efforts here!

I notice from my comments in the AI TMGMT module per #11 that the blocker for merging that is the ability to get unescaped html back from AI Translate. I believe that escaping we cannot opt out of still, but I could be wrong.

Aha I see, comments already there like this, just needs addressing, hence 'Needs work' status. Sorry for the noise.

@scott_euser: I tried to address all you remarks in gitlab:

1. Clearer hook_requirements()
2. Handle case for null source language
3. Model selection on the AI settings screen
3. hook_update() that sets default translate provider when it's empty

Can you check if you still get escaped HTML?

You have test failures: https://git.drupalcode.org/issue/ai-3478721/-/jobs/4148041

Passes now

Tested and translation works without error using the ai_translate module's form. The issue description mentions unifying the provider with the AI CKEditor module as well, but the settings for that still only allow me to set the chat provider for my OpenAI provider, not the spoofed Chat Proxy to LLM provider that I set up in the AI Default settings. Not sure if it's out of scope for this work or not though, which as I say works fine.

I too was confused about why we wanted to have this new provider for translation, until I read the explanation in the MR discussion: it would be good if we could find somewhere to show this on screen, but at the very least I think we need to update the documentation for ai_translate as part of this work.

I've added some remarks, as Paul wrote, documentation changes would be great as well. Like Scott I did also not get the name "Chat proxy to LLM" directly, but I can't really think of a better name.

Still nothing in the way of documentation as to why you would want a provider for translation that duplicates any other you might be using.

@mrdalesmith: The issue this patch tries to solve: out of the box, with AI module alone or AI recipe in Drupal CMS), users have "Translate text" operation type on the settings screen, and the list of providers that implement this operation type, is empty.

Which is confusing, and the most frequent question I get from the new users trying to use AI translation capabilities, is why they cannot use OpenAI or Claude for translation.

By "any other you might be using", I am aware only about deepl implementing "Translate text" operation, and this is not mentioned on the AI settings screen.

I'm aware @valthebald, but not everyone using the module will be: I am suggesting that the documentation for the module in the docs folder needs updating to explain this as part of this MR before it is merged so that everyone can be aware of why this is needed when they come to configure the module.

Changed the documentation of translate text call

That all looks good to me - it probably needs a follow up ticket to then use this new provider with the other AI translation methods, but I don't think that needs doing here urgently. @marcus are you happy with this?

To not duplicate efforts, I didn't re-review the code, looks like Marcus as gone through fairly thoroughly again (and I've seen it in earlier state). I tested this out with ✨ Fallback to ai_translate configuration Active (which is relying on this MR, so I'll merge shortly after this gets merged). I used a handful of contents via batch and reviewed both the results in TMGMT UI + in AI Logging sub-module and can see this is working quite well.

Its a great improvement from AI TMGMT default translation prompt management, thank you!

+1 from me

Removing the 'Discuss' since I think we are well beyond that :)

Lets merge it!

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