When using the layout builder the suggestion does not include blocks created with the layout builder.

We should add the blocks from the full page view mode to the suggestions when creating a job.

Promoting due to the wide adoption of Layout Builder.

Issue fork tmgmt-3097660

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

nod_ created an issue. See original summary.

miro_dietiker’s picture

I agee that this integration is technically possible for an initial translation.

However when i investigated those layout builder content blocks they looked like non-reusable, non-revisionable and technically untranslatable. As a result, any such block added in a translation is disconnected from the original. It is not possible to remap them with additional update translation cycles or detect changes. (Back then they were also lost when you added a translation manually and needed to be manually added...)

Thus the tmgmt promise of "managing translation" is not possible... in fact, in my opinion, the whole feature is technically broken as it doesn't properly integrate with Drupal Core translation workflows at all.

Did something change about this meanwhile? If not, you might want to raise awareness and priority in addressing these problems.
The first initial translation is only a tiny fraction of the problem..

nod_’s picture

I see, it seems we can see even inline blocks in the source list in the custom blocks code list /admin/tmgmt/sources/content/block_content even if on the admin list they don't show up. It feels like it's less dramatic than it seems no?

berdir’s picture

Yes, they are shown there an can be translated manually, but what Miro means is that layout_builder itself does not properly support translations. Until #2946333: Allow synced Layout override Translations: translating labels and inline blocks is resolved, support in TMGMT is going to be very limited. Definitely if you also add content_moderation.

Once core fully supports translations, we could automatically include blocks like we do paragraphs.

nod_’s picture

miro_dietiker’s picture

Priority: Normal » Major
Issue summary: View changes

I hear that
There is a solution that layout builder can contain symmetric translated non-reusable blocks
https://www.drupal.org/project/layout_builder_st
Usage already at 3000+.

One problem is that the block + revision IDs are serialilzed on the layout aware host entity field. So they need to be updated from the host perspective. The block reference to save the block is not enough.

I think that we could "easily" create a plugin that provides these block fields as data items and save them back. And it seems with that symmetric approac, things also won't fall apart?

miro_dietiker’s picture

While the plugin proposal with support for l ayout_builder_st seems pretty straight forward, the impact of combining it with content moderation will need to be tested. It's common that many unexpected things come up with it...

Shawn Conn made their first commit to this issue’s fork.

berdir’s picture

@Shawn Conn: I see you started a merge request to implement this.

I'm not sure if I'd implement this in the way you did. My rough idea was to handle it like paragraphs, so we'd automatically inline the translations into the same job item if they are non-reusable. For reusable blocks, we could support it through the suggestions API and allow them to be added additionally.

shawn conn’s picture

@Berdir first time using the new Gitlab merge fork process as I needed someplace to put a WIP patch for testing. When I created the fork, I wasn't expecting it to open notifications on the page. I guess the behavior is a little different from GH.

Anyway, there are some subsystem parts of TMGMT I'm not familiar so I'd defer to you on what's the more comprehensive solution here. This WIP patch was to expedite a temporary solution until #2946333 unblocks this issue.

Christian.wiedemann made their first commit to this issue’s fork.

kmonty made their first commit to this issue’s fork.

kmonty’s picture

I added Content Moderation support for the WIP Layout Builder patch. While I know based on comment #9 that this patch isn't the approach the maintainer would ultimately like this issue to take, I figured this change could help folks already using the patch.

aspilicious’s picture

Status: Active » Needs review
StatusFileSize
new18.08 KB

The current patch in this issue doesn't update the "master entity". The referenced issue does it after I updated the patch but only for nodes.
THIS patch removes the node specific checking and makes it work for every content entity.
I tested this with a block type that had layout builder support.

aspilicious’s picture

That was the node patch, here is the content entity interface version.

gantal’s picture

I am currently experiencing this issue and tried the patch in #16 but it seems like that patch adds some missing references in TmgmtJobItemSubscriber.php. For example, the TranslatableSectionStorageInterface class does not exist and ContentEntityInterface objects don't have the getKeys method. Is there perhaps another patch that needs to be applied before this one?

duwid’s picture

For everyone using layout_builder_st module instead of the patch from 2946333, here is an updated version of this patch, that will fix the following error message:
Error: Undefined class constant 'TRANSLATED_CONFIGURATION_FIELD_NAME' in Drupal\tmgmt\EventSubscriber\TmgmtJobItemSubscriber->updateSectionComponents() (line 141 of modules/contrib/tmgmt/src/EventSubscriber/TmgmtJobItemSubscriber.php).

aspilicious’s picture

@gantal the getKeys stuff was indeed a mistake.
Here is an updated patch for that line.

phma made their first commit to this issue’s fork.

phma’s picture

Status: Needs review » Needs work

As we have to keep multiple implementations in mind for now, I suggest we use different event subscribers depending on which layout builder translation module is enabled (or in case core has been patched).

public function register(ContainerBuilder $container) {
    $modules = $container->getParameter('container.modules');
    if (isset($modules['layout_builder_at'])) {
      $container
        ->register('tmgmt.job_item_subscriber', 'Drupal\tmgmt\EventSubscriber\AsymmetricTmgmtJobItemSubscriber')
        ->addArgument(new Reference('entity_type.manager'))
        ->addArgument(new Reference('logger.factory'))
        ->addTag('event_subscriber');
    }
    elseif (isset($modules['layout_builder_st'])) {
      $container
        ->register('tmgmt.job_item_subscriber', 'Drupal\tmgmt\EventSubscriber\SymmetricTmgmtJobItemSubscriberBase')
        ->addArgument(new Reference('entity_type.manager'))
        ->addArgument(new Reference('logger.factory'))
        ->addTag('event_subscriber');
    }
    elseif (isset($modules['layout_builder'])) {
      $container
        ->register('tmgmt.job_item_subscriber', 'Drupal\tmgmt\EventSubscriber\CoreTmgmtJobItemSubscriberBase')
        ->addArgument(new Reference('entity_type.manager'))
        ->addArgument(new Reference('logger.factory'))
        ->addTag('event_subscriber');
    }
  }

I'm currently looking into supporting layout_builder_at (wished we would have gone with layout_builder_st or the core patch instead...). The code shouldn't break anything, but it also doesn't update the node yet. It's possible that the block of the source node is being overridden, though, so use with care! I wasn't able to test code with layout_builder_st, so if @aspilicious or @Duwid could check it, would be great!

To @aspilicious and others: Is it possible to stick to the issue fork and not post patches moving forward? It makes it hard to keep track of the changes and merge them back into the fork. You can easily request push access and contribute your changes to the fork.

phma’s picture

layout_builder_at only works, after this patch is applied:Applying patch #3286719: Fails on translated inline blocks for layout_builder_at is highly recommended if you want to also be able to add translations directly without using a tmgmt job. Also, to be able to keep editing the source blocks, the latest patch from #3053881: Reverting entity revisions that contain custom blocks erroneously triggers EntityChangedConstraint is needed. I did notice that strange things start happening when reverting revisions, so watch out.

EDIT: #3053881 might no longer be needed, as I'm also updating the revision number in the latest commit (needs testing). Also, I didn't encounter any issues with loading and reverting older revisions any more.

odai jbr’s picture

The patch works fine with TMGMT but it fails when you have a nested inline blocks as it doesn't create new copies from the children blocks.

johnrosswvsu’s picture

I can confirm that the nested inline blocks are being sent and received but once the entities are saved the nested inline block is not saved. Or if they are saved the wrong revision or target_id is used in the translation.

larvymortera’s picture

Here's an updated patch addressing the nested inline blocks issue.

johnrosswvsu’s picture

Can confirm that the patch on #25 applies cleanly and that the nested (child) inline blocks are translated.

johnrosswvsu’s picture

Please disregard this patch.

johnrosswvsu’s picture

odai jbr’s picture

there is another issue in the patch, a huge number is created from each translated block... after translating a page that has a layout containing 4 blocks only we found that the blocks has been created multiple times, same content and everything but duplicated many times, the layout used the latest translated blocks but all the other duplicates are created for nothing.. we found this after we noticed the site became too slow because of the huge number of blocks..

ahmad khader’s picture

This patch addresses the following issues within the module and the above patches, specifically tested on 'layout_builder_at':

1. Remove Duplicated Inline Blocks.

2. Separate Inline Block Translations with Unique IDs:
Each inline block's translation is assigned a unique ID, intricately connected with the original ID within the configuration. This prevents inline blocks from disappearing in the source language when updating other translations. Notably, the configuration of the component's fixability becomes crucial, as it ensures sensible handling of inline-block translations.

3. Restrict Display of Inline Blocks on Source Page:
Given that inline blocks are inherently connected to entities and cannot be individually translated, there is no need to display them on the source page. This restriction streamlines the visual presentation of the source page.

4. Enhance Functionality of Inline-Block Translations and Nested Inline Blocks:
Bugs have been addressed, and new features have been added to optimize the translation process:

- Blocks not present in the source language can now be retained without disappearing during translation requests.
- Adding a new block in the source language and subsequently requesting translation via TMGMT will only include the newly added block and they will appear on the bottom of the page to void conflicts with other blocks.
- For retranslation needs, simply deleting a block from the translated entity and requesting translation will seamlessly add it again.

It's important to note that this patch has undergone testing exclusively on 'layout_builder_at'. If you have further questions or need additional details on the implementation, feel free to inquire.

ahmad khader’s picture

StatusFileSize
new74.29 KB

support-lb-with-inline-block-3097660-30.patch with a ticket issue in it is the right patch.

ahmad khader’s picture

Fixes to #31 patch

ahmad khader’s picture

StatusFileSize
new74.57 KB

continuation of patches #30-#32 fixes.
This patch addresses the error Drupal\Core\Entity\EntityStorageException: Invalid translation language (es) specified. in

Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (line 817 of core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).
Drupal\tmgmt\EventSubscriber\AsymmetricTmgmtJobItemSubscriber->updateSectionComponents(Object) (Line: 43)
Drupal\tmgmt\EventSubscriber\AsymmetricTmgmtJobItemSubscriber->handlePreSave(Object) (Line: 120)
Drupal\tmgmt\EventSubscriber\TmgmtJobItemSubscriberBase->onAccepted(Object, 'tmgmt.job_item_accepted', Object)
call_user_func(Array, Object, 'tmgmt.job_item_accepted', Object) (Line: 111)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object, 'tmgmt.job_item_accepted') (Line: 1399)

, caused by non-Block content type entities accessing the code.

nginex’s picture

Guys,

Please do not refactor/fix coding standards of non-related code to this issue, you are causing at least a merge conflict and a lot of mess, this is not a scope of the issue, it's a purpose of a separate issue in the project

heddn made their first commit to this issue’s fork.

heddn’s picture

Status: Needs work » Needs review

I've reverted the out of scope changes in the MR. Unfortunately, none of the patch changes since about comment #23 are in the MR. If there are features in those patches that need to make there way into the MR, please post to it instead of a new patch.

heddn’s picture

I think (hope) the JS test failure is a random failure. But everything else is green again.

heddn’s picture

Dug into the last failing test (JavaScript). After running it several times on local, I've come to the conclusion it is a random failure. After all that, it also now runs green on the testbot. The MR is now ready for a round of reviews.

heddn’s picture

Status: Needs review » Needs work

One note about the code in the MR. "Add to cart" does not select layout blocks. Only "Request translation" selects the layout blocks.

marvil07 made their first commit to this issue’s fork.

marvil07’s picture

Status: Needs work » Needs review

One note about the code in the MR. "Add to cart" does not select layout blocks. Only "Request translation" selects the layout blocks.

Indeed, that was the case.

I have added support for that.
Sadly I needed to do that with a work-around.
For some reason the form state value related to layout builder toggle is not available on the cart submit callback.
So, I have added a form state arbitrary value based on the actual value of the toggle, at the validation stage, which is the last I moment I see it there.

nicxvan’s picture

Status: Needs review » Needs work

I just wanted to comment that this only supports content blocks, it would be nice if it supported config blocks too.

I'm working on it locally and will submit it back, but I'm leaning heavily on how lingotek did it here: #3180664: Layout builder symmetrical translations support

Even cutting out the nice symmetrical vs asymmetrical handling locally it seems to pull in the config blocks.
Setting this to needs work, but if that is out of scope for this issue let me know.

nicxvan’s picture

Status: Needs work » Needs review

Having worked with this for a bit, this isn't the right issue for what I mentioned in 42.

I'm doing a review now.

finex’s picture

Hi, I've tried the MR2. It looks promising but I've to report a couple of issues:

Translation of key values should not be allowed

Key field values should not be translated. For example, I have a block with a custom field called "image position" that has three possible values: top, left, and right. The field itself is translatable because, using AT, the image position may vary between languages.

However, MR2 translates the field’s values when it should not, as these values must remain unchanged.

Extreme duplication of records

Another major issue I've encountered is the extreme duplication of records. I tested this on a trilingual website by creating a basic page with a custom layout. The layout contained twelve sections and 26 blocks. Before the translation process, the block_content table had exactly 26 records, one for each block. However, after translating the page into the other two languages, the table grew to more than 40,000 records.

ahmad khader’s picture

@finex,
I fixed the duplication of records on #30-#33 patches with other improvements.
perhaps we should include the patch in the MR!

nicxvan’s picture

@ahmad khader yes please do!

finex’s picture

Thank you @ahmad khader.
I will try the new MR.

camvertiz made their first commit to this issue’s fork.

camvertiz’s picture

When the tmgmt_get_layout_builder_inline_blocks hook was replaced by tmgmt_retrieve_layout_builder_inline_blocks, the changes introduced in #13 were inadvertently lost. I've re-applied those changes to restore content moderation support in the feature.

camvertiz changed the visibility of the branch 3097660-add-layout-builder to hidden.

camvertiz’s picture

Integrated the changes from patch #33 into the merge request (see comment #45). Also the pipeline was failing because the specified Webform versions (6.1.x-dev || 6.2.x-dev) require older versions of Drupal core (up to 10.x), which are incompatible with drupal/core-recommended 11.1.7. I updated the Webform version to fix this.

kmonty changed the visibility of the branch 3097660-add-layout-builder to active.

kmonty’s picture

I'm spinning off the composer issue into its own item here #3524981: Webform version constraints in TMGMT's Composer file are breaking builds

jhedstrom made their first commit to this issue’s fork.

jhedstrom’s picture

StatusFileSize
new48.8 KB

I addressed some PR feedback and am adding a patch file here for use with composer. So far in testing this is working fantastically!

papagrande’s picture

The patch in #55 applies cleanly to both v1.16.0 and v1.17.0. Thanks, @jhedstrom.

hmdnawaz’s picture

I have tested the patch from MR.

I have created a custom block in my module \Plugin\Block\MyBlock, that extends the BlockBase.

I have added that block to the layout builder, but it is not included in the translated version.

Similarly, for block_content. I have added an inline block, and the same applies to this block as well.

In the function tmgmt_retrieve_layout_builder_inline_blocks there is a check if (isset($component_config['block_revision_id'])), the block config doesn't have block_revision_id both for custom and block content and because of that this function returns an empty array.

And one more question: Is this issue only applicable to block_content, and is there no support for custom blocks?

phjou’s picture

The patch from #55 works very well for the most part.

I do have an issue for block titles, when accepting changes, the "Block description" field in the interface doesn't seem to be used and doesn't update the block title.

mpereztejeiro’s picture

StatusFileSize
new667 bytes

The patch from #55 works very well for the most part.

I do have an issue for block titles, when accepting changes, the "Block description" field in the interface doesn't seem to be used and doesn't update the block title.

That was the issue for us as well but we made another patch that introduced this in /src/EventSubscriber/AsymmetricTmgmtJobItemSubscriber.php with a patch:

   protected function updateComponentConfiguration(array $configuration, BlockContentInterface $replicated_block) {
     $configuration["block_revision_id"] = $replicated_block->getRevisionId();
    $configuration["label"] = $replicated_block->label();
     return $configuration;
   }

I post here the patch we came up with to fix this.

kmonty’s picture

Status: Needs review » Needs work

This requires a manual rebase due to merge conflicts.

phjou’s picture

StatusFileSize
new48.09 KB
new3.17 KB

I encountered a bug.

The TmgmtJobItemSubscriberBase::onAccepted() method incorrectly passes the job item's entity ID to InlineBlockUsage::getUsage() regardless of the job item's entity type. This causes unintended behavior when a node ID happens to match an existing block_content ID.

Steps to Reproduce

  1. Have an inline block (e.g., block_content ID 85) used in Layout Builder on a node
  2. Create a translation job for a different node that happens to have the same ID (node ID 85)
  3. Accept the translation for the node job item
  4. The code incorrectly looks up inline block usage for ID 85, finds the unrelated block_content, and attempts to clone its layout sections

So I just added a test to make sure that we are managing only block_content.

    // Only process block_content items; getUsage() expects a block_content ID.
    if ($event->getJobItem()->getItemType() !== 'block_content') {
      return;
    }

Please note I created my patch from #55 since the merge request seems outdated. So the diff file is with #55.

phjou’s picture

StatusFileSize
new47.83 KB
new1.02 KB

Ok I have discovered an issue that was crashing my site when translating pages with blocks that had paragraphs fields.

When using createDuplicate, the paragraph is not saved yet, so only the entity is available. The target_id and target_revision_id don't exist yet and end up with a crash when trying to load the paragraphs using those.

The issue exist in layout_builder_at and there is a patch for it:
#3541778: It's possible a referenced paragraph does not exist, resulting in a WSOD

I've just added support for that issue and also integrated the change in #59

kmonty’s picture

@phjou Can you update the merge request with your changes?

trafo’s picture

Some changes from patch #33 were not added to MR. Notably source_block_id is not set in layout section component and used in `AsymmetricTmgmtJobItemSubscriber`. But it is still used in tmgmt_get_translated_inline_blocks.

vasike’s picture

I'm curious if there's anyone make it work with the latest layout_builder_at release?
including the fix mentioned in the latest #62 patch?

for me ... in "custom drupal instance" it doesn't do a thing about the layout builder data ... empty.

kmonty changed the visibility of the branch 8.x-1.x to hidden.

vasike’s picture

Status: Needs work » Needs review

Ok ... I figure out why the "Asymmetric Translation" won't work out of the box:
- Asymmetric Translation does not assume the content entities used are translatable - have Content Translation enabled
- TMGMT - assumes in its code that usually the content entities are translatable ... including the paragraphs (embedded/referenced entites) or in this case inline blocks ...

So I could use this module and the MR, to translate content with Layout Builder Asymmetric Translation.
But I had to do some updates:
- Enable translation for possible entities that gets in a complex Layout Builder "structure"
and on the MR:
- rebased
- Fix block id identifier for tmgmt_get_translated_inline_blocks helper function
- Fix PHP error ContentTranslateForm submit Exceptions.
- Update AssymeticTmgmtJobItemSubscriber with latest code from layout_builder_at module ... that actually works.
- Extra: some PHPCS fixes.

Probably still needs work ... but for now, let's Review

vasike’s picture

Small update on layout_builder_at integration
I created new task there ... to provide "API" that can be used in other places ... including here
https://www.drupal.org/project/layout_builder_at/issues/3577890

And I tried using in this issue MR in AsymmetricTmgmtJobItemSubscriber and it seems it works ...

Maybe it could be helpful not to maintain other module specific implementations ...

kmonty’s picture

@vasike I reviewed the latest commits and they look good.

Scanning over the PR as a whole, it looks like we're still introducing a number of new SAST majors (particularly \Drupal calls should be avoided in classes, use dependency injection instead), but that seems to be an existing pattern in the module as a whole. Not positive if that is a requirement for a merge or not.

I went back and resolved my two open change requests, as they have been attended to.

vasike’s picture

@kmonty thanks for review/updates on MR
about coding ... this module is still on early "D8 shape", I would say ... so I stopped me continue with such updates as ... the MR would be huge, imho.

As I said in my previous comments ... I only checked Asymmetric Translation, so It would nice to have also checked the Symmetrical one - https://www.drupal.org/project/layout_builder_st

Question ... Wondering if for this issue a similar approach as for EntityReference fields (getEmbeddableFields) could be an option ... then separate job items ...

vasike’s picture

I discovered a problem with Asymmetric translation
AsymmetricTmgmtJobItemSubscriber is running for all inline blocks ... so it runs multiple which means will create/recreate the sections and new inline blocks that leads to some data duplications and maybe also loosing translations done ... from previous iterations as there is some cleanup in the process.

So we need for the Asymmetric Translations to operate at the Job level and not Job item level ... and only on the parent entity and not inline block (content block)

And I pushed a new commit for this refactor.

And another question/issue about this topic ... what happens with entities and their data for entity_reference_revisions in the process.
Could it "dirty the DB" ?!?!

some testing and feedback would help. thanks

yazanmajadba made their first commit to this issue’s fork.

kmonty changed the visibility of the branch 3097660-d11.3.x to hidden.

kmonty’s picture

@yazanmajadba If you have proposed changes to the existing MR, can we just focus the work in MR!2? There does not seem to be a need to propose two different solutions for maintainers yet.

yazanmajadba’s picture

StatusFileSize
new75.22 KB

I tried several patches here, but they caused duplication issues. I refactored patch #33 to work with D11 and added a few enhancements. I tested it, and it worked successfully for me.

hkumar4’s picture

@yazanmajadba your changes doesn't include the latest changes done by @vasike. Can you please add your updates to the existing MR!2?

hkumar4’s picture

Hi @vasike can you please let me know if existing MR works well for async translations or it will need further tweaking.

ao5357’s picture

StatusFileSize
new74.21 KB

I believe this is a correctly re-rolled patch of #76 to cleanly apply to 8.x-1.19