Problem/Motivation

When using external cache and in some regards the Drupal page cache, the language cookie will keep on being set when it was once for a page (e.g. due to it not existing). This because the Drupal cached the setCookie response.
Cookies being set should not be cached however, so this might kill your external cache (e.g. default Varnish behaviour).
To circumvent this, you could tell Drupal not to cache any response that sets a cookie.

But then there is still the problem in Drupal caching a page without a setCookie and serving that to anyone visiting that page without a cookie, resulting in them not getting the language cookie.
This could also be circumvented by altering the default Drupal page cache behaviour, but it starts to get a little complicated.

Proposed resolution

The easy solution to both problems described above (without fancy circumventions) is having this module set its cookie via JS, or at least having the option to.

CommentFileSizeAuthor
#21 language_cookie-client_side_cookie_setting-3193793-9.patch6.93 KBdrubenjamin
#9 language_cookie-client_side_cookie_setting-3193793-9.patch7.03 KBsaso.sotlar
#8 language_cookie-client_side_cookie_setting-3193793-4.patch5.26 KBgilles_webstanz
#6 language_cookie-client_side_cookie_setting-3193793-6.patch5.27 KBcgoffin
#5 language_cookie-client_side_cookie_setting-3193793-5.patch5.26 KBcgoffin
#4 language_cookie-client_side_cookie_setting-3193793-4.patch6.54 KBradelson
#3 interdiff_1_3.txt2.01 KBnginex
#3 language_cookie-client_side_cookie_setting-3193793-3.patch6.75 KBnginex
#2 language_cookie-client_side_cookie_setting-3193793-1.patch6.97 KBl_vandamme

Issue fork language_cookie-3193793

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

L_VanDamme created an issue. See original summary.

l_vandamme’s picture

l_vandamme commented
StatusFileSize
new language_cookie-client_side_cookie_setting-3193793-1.patch6.97 KB

I've already created a patch for this in the version we're currently using (8.x-1.0).

nginex’s picture

nginex commented
Version: 8.x-1.0 » 8.x-1.x-dev
Status: Active » Needs review
StatusFileSize
new language_cookie-client_side_cookie_setting-3193793-3.patch6.75 KB
new interdiff_1_3.txt2.01 KB

I cleaned some debug code and removed usage for ES6 standard, it's still not fully supported in some browsers

radelson’s picture

radelson commented
StatusFileSize
new language_cookie-client_side_cookie_setting-3193793-4.patch6.54 KB

Here is a patch reworking a bit the JS.

It's using the cookie library provided by core.

I also removed the usage of $( document ).ready

cgoffin’s picture

cgoffin commented
StatusFileSize
new language_cookie-client_side_cookie_setting-3193793-5.patch5.26 KB

Here an updated patch that works on 8.x-1.2.

cgoffin’s picture

cgoffin commented
StatusFileSize
new language_cookie-client_side_cookie_setting-3193793-6.patch5.27 KB

There was a bug in my previous patch, here the one without the bug.

radelson’s picture

radelson commented

Patch in #6 doesn't appear to include any Javascript to set the cookie, so that's weird.

Not sure what are the changes made in #5 and #6 apart from removing the clientside code.

gilles_webstanz’s picture

gilles_webstanz commented
StatusFileSize
new language_cookie-client_side_cookie_setting-3193793-4.patch5.26 KB

Hello,

I rerolled the patch #4 for version 8.x-1.2.

saso.sotlar’s picture

saso.sotlar commented
StatusFileSize
new language_cookie-client_side_cookie_setting-3193793-9.patch7.03 KB

Since patches in #5, #6 #8 are either missing JS or contain some old code (or both), rerolled patch from #4 again. It now applies cleanly to latest release version (8.x-1.2) - JS and all required functionality should be there.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Version: 8.x-1.x-dev » 2.0.x-dev
daften’s picture

daften commented
Status: Needs review » Needs work

This needs rework to apply on 2.0.0

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Issue tags: +Needs reroll

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

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll

Rerolled on 2.x

BramDriesen opened merge request !5

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

Issue fork needs a rebase.

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Issue tags: +Needs reroll

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

dieterholvoet’s picture

dieterholvoet commented
Issue tags: -Needs reroll
dieterholvoet’s picture

dieterholvoet commented
Status: Needs work » Needs review
drubenjamin’s picture

drubenjamin commented
StatusFileSize
new language_cookie-client_side_cookie_setting-3193793-9.patch6.93 KB

I've updated the latest patch with two changes to make it work with the latest update.

  • jeroent committed 190cd1e4 on 2.0.x authored by bramdriesen 
    feat: #3193793 Add option to set cookie client side

By: l_vandamme
By:...
jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Status: Needs review » Fixed

Merged to 2.0.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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