🇧🇬Bulgaria @valthebald

@ralkeon: can you please create a merge request? and what is your overall configuration (i.e. steps to reproduce)
Thank you!

We can use ✨ Add text extractor plugins for image and link field types Active as a starting point. With the new base class for field extractors, adding a new one for address field should be quick.
Also, bumping version

I don't think we need to wait until 2.0 to have this feature, downgrading target version

I like the idea, only small changes in MR please

@anjaliprasannan: I don't think so. LB itself has own abstraction layer and does not depend on block_content.
The issue is ai_translate-specific, and should be fixed in LB extraction plugin

valthebald → created an issue.

valthebald → created an issue.

@Aporie nice touch!
Can you please review the messages in https://git.drupalcode.org/issue/ai-3536092/-/jobs/5926826, and also ensure that the code applies to 1.2.x? Since this is schema changing, we probably need an update hook to change existing configurations, especially is this will be backported to 1.1.x

This issue has bothered me "in the background" for some time, only to realize that translations actually can be set to unpublished even when the source entity is published, so this is handled by content moderation module somehow.

So I debugged what happens in content moderation module and added the same code to AiTranslateController.

Please review

LGTM

@marcus created 📌 Move TextExtractor service to AI core Active and assigned to myself. Will try to submit a proposal by Monday EOD

valthebald → created an issue.

@svendecabooter I haven't experienced this, will check

valthebald → created an issue.

@svendecabooter: done! Thanks for pointing this out

Committed to 1.2.x only because of the new method getColumns() in FieldTextExtractorInterface
Thanks everyone!

Copying from Slack:
Having a base class for text extractors sounds good (and it's really easy to add a new plugin!)
One thing I didn't understand is why change FieldTextExtractorInterface?
i.e. passing an entity in extractor constructor instead of setValue(), extract() etc. calls means that we need extractor instance per field per entity, when in the current state, it's extractor instance per field type

@apmsooner: I am back from vacation, and will review this MR ASAP

@balazswmann technically, it does, but you can't use gemini as a chat with tools.
@jibla do you need some help with that?

valthebald → created an issue.

valthebald → created an issue.

Fixed in 1.1.x, 1.2.x and 2.0.x with https://git.drupalcode.org/project/ai/-/commit/26d58a45ce4408afadafec7b6...
and

Also, bumping version - no sense in having it in 1.1.x that is in bugfix-only mode

@apmsoon: good, thanks for clarification!

I feel that we're stuck somehow, would it make things easier to split this issue into multiple (by field type)?

It looks like core bug prevents such behavior - when creating a new translation with enabled content moderation, "published" key is set to all translations based on the "moderation_state" field.

Postponing until 🐛 New translations for moderated nodes are not created in the initial workflow state Needs work is resolved

Closing this since no steps to reproduce and probably fixed elsewhere

valthebald → created an issue.

valthebald → created an issue. See original summary → .

Can't reproduce it with the latest 1.1.x or 1.2.x
Is this still relevant?

After some debugging, the reason for the status being kept as "Published" is \Drupal\content_moderation\EntityOperations::entityPresave() that checks $entity->moderation_state field but not the "published" key (which is changed indirectly.

Needs to be tested how this affects the published status of original entity (it should stay intact)

@efpapado: there could be an option to match both flows, and this is "let the user decide" option in
AiTranslateSettingsForm::buildForm (see the second MR):

      '#options' => [
        'keep_original' => $this->t('Keep the status of original entity'),
        'create_draft' => $this->t('Create translation in draft status'),
        'user_decision' => $this->t('Let the user decide'),
      ],

but in any case, non-unpublishing the translation is the bigger target to hit

@efpapado: we could argue on possible reasons to change the flow, but there's more:
1. UI change - "Translate using ... " already takes significant part of the translation list screen, if we have 2 options instead of one, the screen would look like this:

This doesn't seem an improvement of UX. Maybe those options can go to the actions list (Add/Edit/Delete)?
2. More importantly, "force save unpublished" doesn't take any effect, I still get a published entity if the original entity is published.

Also, bumping target version

I see some common approach with how AI translate module extracts textual data from (sometimes nested) entity fields - there is FieldTextExtractorInterface introduced in https://www.drupal.org/node/3464024 → and TextExtractor service that has extractTextMetadata/insertTextMetadata methods.

Can this be reused in content suggestions?

That would require some (minor) changes like moving the service and field plugins from ai_translate to ai module itself, but benefit IMO is bigger

@efpapado: oh, I didn't realize that.
Still, I'd prefer the approach in https://git.drupalcode.org/project/ai/-/merge_requests/696 - I don't think it's the site builder who decides what is the translation flow (i.e. is the AI-translated page gets published), but the site administrators.

valthebald → created an issue.

kristen pol → credited valthebald → .

@efpapado: I'm afraid your approach creates an option to permission bypass, i.e. by manually adding 1 to the link, user will be able to publish an entity even if they don't have access to

I have the same behavior as @anjaliprasannan

@svendecabooter definitely related! Thanks for pointing out

valthebald → created an issue.

Merged to 1.1.x and 1.2.x, thank you all!

valthebald → created an issue.

To summarize the discussion in Slack, it would be good to not hard code the skip list of possible column names to (delta, parent, etc.)
Possible solution to check special key of "_translatable_columns" (name discussable, could be "_translatable_fields" or simply "_columns")
in TextExtractor::extractTextMetadata():

        foreach ($fieldMeta as $meta) {
          $fieldParents = [$field_name];
          if (isset($meta['delta'])) {
            $fieldParents[] = $meta['delta'];
            unset($meta['delta']);
          }
          if (isset($meta['parents'])) {
            $fieldParents = array_merge($fieldParents, $meta['parents']);
          }
          $meta['parents'] = $fieldParents;
          if (!isset($meta['_columns'])) {
            $meta['_columns'] = ['value'];
          }

and then loop over "_columns" in AiTranslateController::translateSingleField()

Merged to 1.2.x and 1.1.x, thank you all!

valthebald → created an issue.

LGTM

@all: does it mean we're good to go? This seems to be the last blocker to release 1.1.0

LGTM
I have searched the codebase for "\Hook" and found only one hook class other than ai_content_suggestions - ai_test.

Should there be a separate issue for it?

valthebald → created an issue.

Committed to 1.x, thanks everyone!

I believe this is solved by https://git.drupalcode.org/project/ai_provider_google_vertex/-/commit/97..., can we close the issue?

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

valthebald → created an issue.

Actually, it is possible to perform the check earlier and avoid the ReferenceFieldExtractor::extract() call

@lakhal: thanks for reporting this. The issue is caused by ReferenceFieldExtractor assuming that in the entity reference field, referenced entities are (fieldable) content entities, which is not the case for config entity Domain.

Possible solution is to check that $subEntity is instance of ContentEntityInterface before calling extractTextMetadata($subEntity)`:

    foreach ($entity->get($fieldName)
      ->referencedEntities() as $delta => $subEntity) {
      // Intentionally check each sub entity because of https://www.drupal.org/project/drupal/issues/2407587
      if (!($subEntity instanceof ContentEntityInterface)) {
        continue;
      }
      foreach ($this->textExtractor->extractTextMetadata($subEntity) as $subMeta) {
        $textMeta[] = ['delta' => $delta] + $subMeta;
      }
    }

Also, if you switch from 1.0.5 to 1.1.0-beta, you can disable domain translation and avoid this issue (go to Administration/Content types//fields/:

Version 1.1 is not BC with 1.0.5, but there will be an upgrade path.
If you start building a WIP, better start with 1.1-beta

@anjaliprasannan I've added some code style comments

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