Problem/Motivation

Right now, if big pipe is enabled, the widget can end up rendering twice. It also has caches explicitly disabled on the block which causes problems on pages where the block is used when big pipe is off (basically pages are uncacheable).

Steps to reproduce

Proposed resolution

Refactor the module to use Drupal libraries and settings to render a cacheable block and avoid using the placeholder with big pipe (since the block should be cacheable).

Remaining tasks

I've rewritten most of the module (including removing some D7 hooks which were left over). There's likely more refactoring that could be done in the build() method in the block plugin.

User interface changes

None

API changes

Most of the functions in the block plugin file were removed in favour of a library alter and settings alter. And each cdn and local js file has their own libraries.yml file entry. There is also an init js file now to set the window.gtranslateSettings variable.

Data model changes

Everything is the same as it was other than the data-* attributes on the script tag when including the gtranslate js. The domain is still set but the path is omitted (to allow caching and avoid having scripts create script tags). But the gtranslate js code already figures out the path on its own anyways so there should be no loss in functionality.

CommentFileSizeAuthor
#26 3346829-refactor-for-bigpipe-26.patch77.76 KBo_timoshchuk
#20 3346829-refactor-for-bigpipe-11.patch47.22 KBmmaranao
#19 error-MR-11-no-block-inline.png12.73 KBsocialnicheguru

Issue fork gtranslate-3346829

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:

About issue forks

Comments

minorOffense created an issue. See original summary.

minorOffense opened merge request !11

minoroffense’s picture

minoroffense commented

Just to add a bit more clarity, because the translate js is currently not wrapped in a behavior, when the script tag gets rendered, it creates the gtranslate tag too and that code immediately runs on the page. So if something ajaxes something in or refreshes a block or region, drupal can trigger that js to run again. And so you end up with two translate drop downs.

By allowing the block to be cached and telling Drupal to not create a placeholder (see https://git.drupalcode.org/project/gtranslate/-/merge_requests/11/diffs#...) for that block, it won't let big pipe bring it in.

But by doing so, we want to ensure that block can be cached otherwise you break all caches on any page that block is on.

So we remove most of the work in the block template, use libraries properly and add a settings_alter for whatever logic needed to run for that block.

socialnicheguru’s picture

socialnicheguru commented
Status: Active » Needs review
socialnicheguru’s picture

socialnicheguru commented

the MR applies cleanly.
How can I test?

socialnicheguru’s picture

socialnicheguru commented

is there a way to include the js locally if wanted?

I see that there is a CDN option that can be unchecked

socialnicheguru’s picture

socialnicheguru commented
Status: Needs review » Needs work

my gtranslate Block no longer shows up in my theme.

I have cleared cache
there are no JS errors
Drupal 9.4.12
php 7.4.x

I had to remove the Block then re-enable each time I make a change to the gtranslate settings page.

Block placed on page

The inline did not seem to work.
The other settings seemed to but it seemed sporadic. Sometimes the block would show and sometimes not

minoroffense’s picture

minoroffense commented

Probably a cache tag needs to be set or invalidated when a setting is changed if that's the case. I'll have a look.

laurentd’s picture

laurentd commented

Nice work, much cleaner code!

Noticed some wrong mappings of the widget look options (e.g.: dropdown_with_flags) and their corresponding service names (e.g.: widget_dwf) in libraries.yml.

Proposed change:

cdn_widget_dropdown_with_flags:
  js:
    https://cdn.gtranslate.net/widgets/latest/dwf.js: {
      type: external,
      minified: true
    }
  dependencies:
    - gtranslate/init
socialnicheguru’s picture

socialnicheguru commented
Related issues: +#3342690: Is the gtranslate.libraries.yml supposed to be empty?
socialnicheguru’s picture

socialnicheguru commented

@LaurentD

I do not see cdn_widget_dropdown_with_flags in the MR. Are you saying it should be added? If so what else should be added?

socialnicheguru’s picture

socialnicheguru commented
Related issues: +#3214197: Asynchronous Google Translate

@minorOffense

The gtranslate block cannot be cached. Is this correct?

'#cache' => array('max-age' => 0), is set in gtranslate/src/Plugin/Block/GTranslateBlock.php

Should it be cached per role or per site?
Should per site be best since as of now, the settings do not change per user.
I am currently using it sitewide per theme.

In this issue and related makes a great point about changing the cache to permanent #3343751: Block content cache 'max-age' for Gtranslate 3.0+

This is also interesting #3214197: Asynchronous Google Translate

socialnicheguru’s picture

socialnicheguru commented
Related issues: +#3343751: Block content cache 'max-age' for Gtranslate 3.0+
minoroffense’s picture

minoroffense commented

As far as I can tell since everything happens clientside the cache on that block should almost never expire. Only if the config changes or a CR. THe markup for the block is the same regardless.

I removed the cache set in my merge request ( I should have remove the commented out code too so I'll have to do that).

// '#cache' => ['max-age' => 0],

The markup is fixed, just the attached libraries change (which Drupal takes care of separately).

I should probably add the cache tags and contexts to the block so it clears when config changes. (I can see my notes from a month or two ago about doing so, haven't had time yet :-/)

minoroffense’s picture

minoroffense commented

@LaurentD Thanks.

Yeah there was a lot of copy/pasting to build out that library. I may have messed some of the dependencies up. I'll check again.

socialnicheguru’s picture

socialnicheguru commented

Block is not showing after I applied the patch.
do i need to totally uninstall.
I did do a drush cr and even cleared cache from the UI

minoroffense’s picture

minoroffense commented

The config changes made the block not show up at all with an upgrade. I had to do a full reinstall going from the 2.x to 3.x. I don't remember if I needed to do a full install between the 3.x versions.

socialnicheguru’s picture

socialnicheguru commented

There is something wrong.
I had 3.0.x without the MR and all was good in the world.
I applied the MR and the block disappeared
I uninstalled and installed and the module and Gtranslate showed for a brief time.
Then I changed the settings and it disappeared.
I cleared the cache.
Now no matter what I do I cannot get the block back.

Maybe add the cache invalidation when the config changes would help?

Right now, I have no gtranslate showing on my pages.

socialnicheguru’s picture

socialnicheguru commented
StatusFileSize
new error-MR-11-no-block-inline.png12.73 KB
mmaranao’s picture

mmaranao commented
StatusFileSize
new 3346829-refactor-for-bigpipe-11.patch47.22 KB

Great work! I've attached the patch here while waiting to be merged.

utcwebdev’s picture

utcwebdev commented

The patch no longer applies in GTranslate 3.0.2. Is this issue resolved now that the module allows block caching?

sseto’s picture

sseto commented

+1 on the patch no longer applies to 3.0.3. Had to revert back to 3.0.1.

sassafrass’s picture

sassafrass commented

We are experiencing this issue as well. I applied the patch to 3.0.1 successfully. However, as others have noted, the block content was no longer displayed. I tried completely uninstalling, deleting all blocks and config, removed the module and code completely from my code base, and then reinstalling, applying the patch, and re-configuring, but the languages still were not displayed. We reverted everything and applied a modified block preprocess function (see: https://www.drupal.org/project/gtranslate/issues/3396734#comment-15402378) to remove the duplicate block content being generated.

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

jmoreira’s picture

jmoreira commented
Status: Needs work » Needs review

Patched rebased so we can use it with new security update. Works well for me. Someone else please review.

o_timoshchuk’s picture

o_timoshchuk commented
StatusFileSize
new 3346829-refactor-for-bigpipe-26.patch77.76 KB

I have created a patch from the merge request to apply it to the module version 3.0.5

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

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

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented

Not sure how people got the patch to apply with the broken line endings, but that should be fixed now.

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Going to set this as RTBC, my 2 limited commits don't really change anything. We're using this on a project of ours and confirm this fixes the Big Pipe issues and the fact it uses libraries properly.

andybroomfield’s picture

andybroomfield
they/them
commented

I'm testing the MR as a patch. I'm using the Popup for the widget look and I see this in console

popup.js?tlkfed:111 Uncaught TypeError: languages.forEach is not a function
at popup.js?tlkfed:111:15
at popup.js?tlkfed:231:3
(anonymous) @

I don't see the language switcher at all, just the wrapper div.