🇧🇬Bulgaria @valthebald

@udhaya kumar: we're almost there!

One note: directly sending response in the form is not the best solution. Could it be something like

<?php
    // @todo Exceptions should not be used for code flow control. In order to
    //   resolve this issue properly it is necessary to completely separate form
    //   submission from rendering.
    //   @see https://www.drupal.org/node/2367555
          throw new EnforcedResponseException($response);

?>

instead of $response->send()?

That way you give more control to Drupal to handle the exception / override form behavior

valthebald → created an issue.

I would like to opt-in this module: https://www.drupal.org/project/ai_translate →
It has 9 issues, and all maintainers agree we are ready to try GitLab issues. We understand we cannot revert this change and there may be some unexpected issues as early adopters

@marcus_johansson: creating stable versions of removed submodules is a not a requirement strictly speaking, but is a very nice to have.
To avoid the situation when (stable) AI 1.3.0 suggests installing non-stable versions of submodules.

I don't have a good solution right now if there's an issue in d.o. composer settings (see ✨ Change namespace for Field Widget Actions module Active , we can really benefit from pushing this forward).

As an alternative, I was thinking about defining submodule URL's in AI's composer.json (as packages), but this needs somehow to propagate to the project's composer.json but not module's one (can wikimedia merge plugin help here? https://packagist.org/packages/wikimedia/composer-merge-plugin)

Having a prompt type selection with a single (and disabled!) select box could be confusing as well...
What if we follow the pattern used in core (check node.module as an example) - if there is only one content type, i.e. page, and you go to /node/add, you're automatically redirected to /node/add/page
How does this sound?

@scott_euser: ai_translate keeps original name (same happened with stable and seven moved outside of the core).
What happens is that the project having less nesting wins, i.e. if there are ai/modules/ai_translate and ai_translate, the latter wins.
So even if you have a submodule inside ai/modules, it is not being used

I've added a reference implementation for the "core way" in 🌱 Deprecate AI translate in the current stable branch Active

How this works:
with applied patch, no drupal/ai_translate in the system (default state for existing installations):

with ai_translate present in modules/contrib/ai_translate:

valthebald → created an issue.

@marcus_johansson: I'd mark the issue as closed, because Alex is already in MAINTAINERS.txt (in 2.0.x and 1.2.x branches), just not sure what's the right status: Closed (works as designed) or Fixed :)

valthebald → created an issue.

Potential blocker that I can think of is finding a security issue in 1.0.x
Yes, we have the upgrade path from 1.0 to 1.2 tested in 📌 Do upgrade testing of AI 1.2.0-dev from 1.0.4 Active , but that covers only ai itself and some well-known providers.
Re: 1.0.x share don't forget IE support was dropped when it's usage has fallen below 2.5% or in some cases even 1%...

valthebald → created an issue.

Other AI sub-modules that have the same issue:

* Content suggestions ✨ Move out AI Content Suggestions Active
* Translate ✨ Move out AI Translate Active
* Validations ✨ Move out AI Validations Active
* Search ✨ Move out AI Search Active

valthebald → created an issue.

Valery "valthebald" Lourie
Mostly figuring out the way towards AI module 2.0, and whether we need 1.3 in the middle (we probably do)
Besides refactoring, some submodules will move in and some move out to align with the different development pace.
Development-wise, I'm focusing on AI translate https://www.drupal.org/project/ai_translate →

@lpeabody: I believe that would be another issue.
IMO, getting rid of thread ID in page HTML is something that should happen no matter which storage to use

@igor mashevskyi: note that there is also DeepChatFormBlock plugin (id="ai_deepchat_block"), and that one is used in CMS recipe, so probably the same changes should be done in block.settings.ai_deepchat_block.schema.yml

Categorizing

I checked the core, and see usage of enforced dependencies only in test modules (so, not merging MR yet). This won't make any harm though.
On the other side, having prompt reference in the config is the right thing to do (so RTBC).
Bottom line, I'll let this to decide for someone else :)

I reviewed related issue in ai_translate, this issue's MR follows the same logic.
Marking RTBC

Back to needs review after rebasing on the latest 1.2.x

@bbruno: I have fixed merged conflicts after rebasing on the last 1.2.x (there were less that I thought), and checked that the chat works in toolbar (anonymous and admin) and "regular" position (anonymous only because of permissions)

@svendecabooter: oops, replacing the prompt text is a good catch :)
regarding enforced dependency - isn't it added automatically by core?

Merged to 1.2.x, thank you @svendecabooter!

I checked fresh install, as well as update from 1.2.x-dev, translations work as expected.
I have some thoughts about optimizing prompt selection UI, but this is definitely not a release blocker.

I tried a fresh reinstall of ai from the latest 1.2, and then switched to the feature branch.
This revealed another issue - prompt type should be installed (if not exists) in hook_update as well. Otherwise there is
"No AI Prompt Types exist. These are normally provided by modules within the AI Ecosystem" error when trying to edit a prompt

Update hook worked like a charm, thank you @svendecabooter!
No strong opinion here, but do we need to keep both default and modified prompts? In the end, we have the default one in module's install folder for the reference

That greatly depends on the target release date for 2.0 and planned features that haven't found their way into 1.2.0.
There is sense in 1.3.x if all the following is true:

1. We can't do all the refactoring/basing on symfony components in ~3-4 months
2. Within the same 3-4 months period, there features that are an easy and/or big win, but break BC

LGTM

valthebald → created an issue.

Merged to 1.2.x, thanks @nikro!

@marcus_johansson: Core has https://www.drupal.org/about/core/policies/roles-and-responsibilities/pa... → exactly for that - pay respects to people who were maintainers, but have stepped down

Took a bit longer than 30 minutes, but PoC is fully working (see the MR). Summarized changes:

1. Remove thread_id from page' drupalSettings
2. Removed thread setting logic from DeepChatApi::setAssistant() (good that there is already DeepChatApi::setThreadsId()
3. Move historicalMessages() method from DeepChatFormBlock to DeepChatApi (because history is AJAX-loaded)
4. Create JS class deepChatSession that handles all the logic of initialization/retrieving the history/resetting etc.
5. Every instance of deepChatElement in deepchat-init.js gets an instance of deepChatSession
6. Changed input and output parameters of DeepChatApi and ResetSession controllers

Open issues/questions

1. Current approach (before or after switch to local storage) assumes there is only one chat session on the page. In theory, it is possible to pass unique storage ids to deepChatSession instances, thus working with different assistants/thread. Is this needed?
2. Is there a need in anonymous session protection other than random 20-character string? Maybe store also user ID, to protect authenticated sessions from hijacking? Can currently unused deepChatSession.secret variable be of any value?
3. Should the history be stored in local storage (all of it or limited number of recent messages)?

valthebald → created an issue.

@svendecabooter: thank you, merged!

valthebald → created an issue.

Here's the concept (in MR 907):

1. Bind thread_id to a random secret string, stored on client side
2. Do not store thread_id in drupalSettings (cache killer!). Instead, use chatSettings JS object, serialized to browser's storage
3. When there is no thread_id in chatSettings, call getSession() to generate fresh one.

The code in its current condition works and doesn't start a session.
What's missing is security check (need to prevent session hijacking)

Changing back to Needs review for wider visibility

LGTM as well

There are some merge conflicts, I wonder if it'd be easier to open a new MR or fix them?

@lpeabody:

Just to be clear, a session cannot exist at all if the page cache is to be used.

Totally, that's why session/local storage accompanied by expirable key/value storage

I will push to the issue branch later today

@svendecabooter: I was wondering the same question!
While I don't know what is the "desired" behavior, the reason I decided to go with the OR logic is that it seems more flexible, i.e. you may want to give certain roles permission to only create translations using the core flow, and some other roles to only create translations using AI.

As an alternative to browser storage (local or session), here's another trick that was used in a semi-decoupled Drupal project: the whole conversation is stored on server side (as it is today), and there's conversation id that is stored in session/local storage of the browser. It is also possible to protect the conversation with randomly generated secret, stored by KeyValueExpirableFactory service.
That way:

• Nothing is stored in the session, so page cache is not broken
• Conversation is protected with a mechanism similar to session generation in Drupal
• Conversation itself is not stored on client side, so lower chance to face clientStorage limitations

Closing as outdated, no evidence this bug exists in 1.2.x, and no feedback from original reporter

This fix accompanies the one from ✨ AI Translate Module Leaves Fields Empty When No Translation is Found Active .
I have search the code base and don't see any calls to addTranslation() that don't get values from original entity.
Thanks everyone, merged to 1.2.x!

Starting review

@mkalkbrenner incubator module is a good mode, reminds me of solrlog that found its way into searchapi_solr :)

After a quick check of https://git.drupalcode.org/project/permissions_by_term/-/tree/3.1.x-dev/..., there's no file AutomatorPluginDeriver.php
Which version of permission_by_term do you use? Any chance there was local override?

Setting the component

@svendecabooter: it's always a tricky question for contrib-contrib plugins :)
I personally don't have a strong opinion here

PHPCS reports some code style issues, please check https://git.drupalcode.org/issue/ai-3541883/-/jobs/6569528

LGTM and the test bot is happy

valthebald → created an issue.

valthebald → created an issue.

valthebald → created an issue.

Option to set the default state for the new translation was already added

@anmolgoyal74: that's a good proposal! Do you want to create a merge request, or someone else can?

I've reworked original idea, and added the logic to hide "AI translations" column when user doesn't have permissions or all languages are already translated

valthebald → changed the visibility of the branch 1.0.x to hidden.

I have added "beta blocker" tag to all the mentioned issues, feel free to add tag/untag/add more issues