Seems this is already fixed in dev, but needs a new release! For that reason I'll keep this open to inform the maintainer. No code changes needed IMHO.
Could you please turn this into a MR?
Thanks @smustgrave!
Do we have any chance to use Drupals library system finally and not include tour.js into the module?
Alright, thank you @berdir! Let us know when we can help then!
I guess we should use
cookieless_mode: 'on_reject',
and then call
a) posthog.opt_in_capturing() if consent is explicitly given
b) posthog.opt_out_capturing() if consent is explicitly denied
But if I remember correctly, the methods in the cookies js are NOT called explicitly but on every page load (either / or)? If that's the case, let's discuss how we can do it explicitly! Is there maybe a cookies variable we can ask if the cookie banner is still unresolved?
Maybe we need to refactor the posthog_cookies part to use the new functionality from
https://posthog.com/tutorials/cookieless-tracking
At least we should try if that fixes the behavior, instead of changing persistence?
Thank you very much @avpaderno! I'll now also make Berdir maintainer and add @Grevil as co-maintainer!
Thanks all! Please note that on the two mainly affected projects we don't use Klaro but COOKiES (which we're planning to replace by Klaro).
We created this issue in both - Klaro and COOKiES because both set
$script['preprocess'] = FALSE;
in hook_js_alter() dynamically.
So I just want to point out that we're not 100% sure this issue exists in Klaro, just wanted to ensure that's checked for this widely used module, because the underlying core "issue" is hard to find (and know about).
The result here could also be that klaro is fine!
Yes I think we should treat this as duplicate of 📌 Automated Drupal 11 compatibility fixes for view_modes_display Needs review
Thanks. Now the active marker class still needs to be set on the variation, right?
FYI: I think this is still valid. Asset injector is widely used and like the COOkiES integration it would also make sense to integrate with Klaro as replacement to allow blocking the asset_injector defined scripts?
Just hadn't had the time yet to look at this in depth. Happy to assist, if anyone likes to start!
@jan kellermann I think what catch wants to say is, that Core can not handle these dynamic alteration of libraries, like done here and in Claro, see the Core issue. It leads to the aggregate errors reported there.
Once I become maintainer, I'll also add @berdir as maintainer. If @btopro replies here, it would be great, if he could also consider Berdir!
I think for the asset_injector submodule (which is also affected!) we might be able to remove the preprocess = FALSE
https://git.drupalcode.org/project/cookies/-/blob/2.x/modules/cookies_as...
because we there already disable the "preprocess" option in asset injector, see
The COOKiES service this asset belongs to (check your existing services here). You may add a new service here, if this is a filter for a not yet existing service. Also you may need to add further information to the service description for this use-case. Note, that js preprocessing is NOT compatible with the COOKiES service!
The "Preprocess JS" setting is disabled and can't be enabled.
Adding the similar Klaro issue: 🐛 Setting "preprocess" false in "klaro_js_alter" could lead to unintentional problems with file aggregation Active
Thanks @svendecabooter yes of course I did so! :)
Thanks @smustgrave. Yes the test-only MR was just there to show the existing issue front-up. Regarding the failing tests: Yes, it will need discussion here how we'd like to resolve this.
Could you or someone else please look into my comment here: https://www.drupal.org/project/drupal/issues/3372446#comment-16332410 ✨ PlainTextOutput::renderFromHtml() could better handle spaces Active and decide how and where to proceed?
The result of the bug are broken sentences / words, which is especially a highly relevant issue for the Metatags module, but of course also any other code that relies on this.
Thanks @miroslavstankov! That sounds great! Would you also like to address 📌 Add basic install tests, gitlab CI, Tugboat and composer.json with dependencies Active maybe? Then we'd have a cleaner start here!
Maybe the module or other dependencies are not enabled? Or for any other reason the module doesn't offer that route?
VERY nice @lrwebks!
Did you see this? https://www.drupal.org/docs/core-modules-and-themes/core-modules/jsonapi... →
@alexpott RE #113: Yes it's still really relevant - our case are menu items with paragraphs (for mega menu) which can't be rendered for anonymous users without this patch. See #2925283: Additional fields in menu items not visible for anonymous users →
We'll rebase this one, but would be really great to finish this then?
I guess we should combine the resolution with 📌 Ensure COOKiES runs last for libraries Active to ensure we have no other side-effects?
Whao, this is super cool @miroslavstankov! Maybe you'd like to open and resolve a further issue to fix eslint, cspell an the others separately? We're going to review this soon!
Thanks, please also fix cspell and eslint if easy. Tests currently fail.
Would be a good thing maybe for AI? What do you think @grevil? (Please review and then let's decide to merge this and create a separate issue for (ai generated) tests.
@lrwebks as of https://stackoverflow.com/questions/978061/http-get-with-request-body I think the implementation might be correct. Does it work and do we have a tests now to check that it works? (We didn't see it broke before, right?)
Last point (adding that class to indicate the type) is for @thomas.frobieter - afterwards, I think this is good enough to go! Thanks!
PS @catch should we also use this to have a look at 🐛 "Only file JavaScript/CSS assets can be optimized" errors in logs Active which seems at least heavily related to the discussion before? Think it would make sense to solve both now?
#7 is still unresolved. I left some comments for better architecture and namings.
This is NOT an issue in google_tag module! It's the linked core issue in combinatin with COOKiES / Klaro module!
See
https://www.drupal.org/project/drupal/issues/3416508#comment-16340916
🐛
Only file JavaScript assets with preprocessing enabled can be optimized.
Active
for details.
The attached patch does NOT fix it. So I'll close this one. Having { preprocess: false } WITHOUT , minified: true is totally fine!
@catch thanks!! And I guess core won't be able to handle this in the near future as-is? (I mean, this is also risky for other cases where people might do this).
@grevil could you please create an issue in both modules (if not yet existing in klaro) and link to #58 there?
I'll try your MR (#59 as static patch) in both projects now. If anyone is fine with it, should we RTBC it or how to proceed here best?
@lrwebks Good enough so far to demonstrate the bug. Let's wait for further feedback now, here or in the other issue.
@catch thanks! No advagg. But you took me on the right road to demystify #51: https://git.drupalcode.org/project/cookies/-/blob/2.x/modules/cookies_gt...
And probably that's the root cause for the edge-case here, besides stale library urls: We're using COOKiES on both projects and COOKiES does the following: https://git.drupalcode.org/project/cookies/-/blob/2.x/modules/cookies_gt...
/**
* Implements hook_js_alter().
*/
function cookies_gtag_js_alter(&$javascript, AttachedAssetsInterface $assets) {
$doKo = Drupal::service('cookies.knock_out')->doKnockOut();
if ($doKo) {
$module_path = \Drupal::service('extension.list.module')->getPath('google_tag');
$scripts = [
'gtag' => $module_path . '/js/gtag.js',
'gtag_ajax' => $module_path . '/js/gtag.ajax.js',
'gtm' => $module_path . '/js/gtm.js',
];
foreach ($scripts as $key => $script) {
if (isset($javascript[$script])) {
$javascript[$script]['preprocess'] = FALSE;
$javascript[$script]['attributes']['type'] = CookiesConstants::COOKIES_SCRIPT_KO_TYPE;
$javascript[$script]['attributes']['id'] = 'cookies_gtag_' . $key;
$javascript[$script]['attributes']['data-cookieconsent'] = 'gtag';
}
}
}
}
and here we go!!
Klaro (part of Drupal CMS) does similar:
https://git.drupalcode.org/project/klaro/-/blob/3.x/klaro.module#L85
/**
* Implements hook_js_alter().
*
* Handles script files added from libraries.
*/
function klaro_js_alter(&$javascript, AttachedAssetsInterface $assets) {
// @todo The assets are cached based on theme, library, langcode etc. so we
// cannot differ based on user permissions.
// @see \Drupal\Core\Asset\AssetResolver::getJsAssets
/** @var \Drupal\klaro\Utility\KlaroHelper $helper */
$helper = \Drupal::service('klaro.helper');
if (!$helper->hasAccess() || $helper->onDisabledUri() || !$helper->getSettings()->get('auto_decorate_js_alter') || !$helper->consentManagementRequired()) {
return;
}
foreach ($javascript as $path => &$script) {
if ($script['type'] !== 'setting') {
$app = $helper->matchKlaroApp($path);
if ($app) {
$script['preprocess'] = FALSE;
$script['klaro'] = $app->id();
}
}
}
}
because both modules need this code to knock-out and identify libraries.
One more thing I just found out: All these errors triggered last night were from Google / Microsoft crawlers. This could be a coincidence, but it could also have a specific reason, such as old entries in the search engine cache / index.
Both files have in common that they are dynamically added in hook_page_attachments(). Might that be a reason, perhaps?
Thanks @catch. What my debugging says is kind of crazy and would IMHO mean we have a more general bug:
It logged:
Only file JavaScript assets **with preprocessing enabled** can be optimized. File: "modules/contrib/google_tag/js/gtag.ajax.js", Type: "file", Preprocess: FALSE"
but gtag.ajax.js has no flags at all:
gtag.ajax:
js:
js/gtag.ajax.js: { }
dependencies:
- core/drupal.ajax
Here's the full google_tag.libraries.json:
https://git.drupalcode.org/project/google_tag/-/blob/2.0.x/google_tag.li...
Am I missing something or does that look like there's something wrong with the aggregation in general?
Otherwise, I would agree with #47 that it's not the default case we're talking about here. The aggregate URL is definitely not manipulated (by us) in our case, so and old aggregate would make sense, especially because we're seeing more of this by time.
This location is logged with the error:
https://www.example.com/sites/default/files/js/js_Tn3zCv2BVTTEfmbwa6nvePuocXuQ5CnPChmGfXdWQn0.js?delta=5&include=eJyFkV2WwyAIhTeUxm3MLjio1JAa8YD9mVl9Tdtpmz7MvAD3-ol4QDNqwGWm0ETdbGPTozWKNkk1uAmPMdHgWbEiBFkW0kCALXRR9qwLNpbiwkThsLFgITNMna08-Iw_386zjDjjpUvp9Fq6NcBN2_BqHyM0gYDaPp75j-g95MBk7pFn018LcPvdfs2otC8sMdOL2mMg32uofKHstvJJpYbJraE7SutcVUrvZmNUOWfwaLTbFTwNL-08hkNSOZa4Wygybs-M3nXUY8X8cMLEOb4jFRVTX8lk4M2lLL6zf0-eRFImeM79oe-LqWJtkgSzOS7chhPT2dwt3oEz-b3o4h557L_Ikq6YXeYT&language=de&scope=footer&theme=my_custom_theme
Thanks for the quick reply @jsacksick - can you already confirm the issue? Might still be something special for our case or the module combination.
BTW how did you disable the response validation? I was looking for a simple way through configuration / settings but didn't find one.
We're seeing this on same backtrace as written in #16 so it's definitely happening. I agree, the question is where and why. The issue summary reports this from google_tag: 🐛 Only file JavaScript assets with preprocessing enabled can be optimized. Active . Maybe it's another contrib module that causes this?
I was already able to find out that it doesn't happen on each page load and not even after clearing caches, so maybe this happens in a race condition or whatever?
The question is: Would it make things worse to also sort out such cases here (once we're sure we understand what's happening). We could then leave a comment in code, why it's necessary (if it is)...
I've added some more logging here in the affected projects, so hopefully we'll know more about it tomorrow.
Ugly hotfix attached to entirely disable JSON:API response validation until this is fixed (MR!13739).
I'll now mark that one as draft to delete once we have a working solution here.
We just ran into a case where it would have been great to have this to disable response validation. I rebased the MR.
The change record looks correct already?
Hi @dsnopek thanks for the quick reply! Yes, Co-maintainership is also fine for me. Could you also add my teammate @grevil then, please?
Our focus should be 📌 Automated Drupal 11 compatibility fixes for ckeditor_drupalbreaks Needs review and ✨ Develop an upgrade path from CKEditor4 to CKEditor5 plugin Active
For CKEditor 4 and Drupal 10 we should then use a separate branch, I think? But please note that you already merged 📌 CKEditor5 Drupalbreaks update Needs review so the 3.0.x is already for CKEditor 5 (the only missing piece is the Drupal 11 compatibility version in .info.yml).
Would you mind creating an issue for a separate D10 or D11 branch? So we don't use this issue for the further steps?
Thank you!
Please mind that any markup change will be a breaking change. And please use MR, not patches.
IMHO this should be solved in the template.
GREAT work @grevil! Works like a charm! Merging!
Yes it's not related to this module, but a core issue that happens in combination with several other modules. Should not be handled here.
This is important to keep things working for people upgrading.
See #17 📌 CKEditor5 Drupalbreaks update Needs review here.
I still see thousands of unrelated code style fixes.
I still see thousands of unrelated code style fixes.
@eglaw thanks, could you please prepare this as MR instead of a patch?
Is this backwards-compatible?
Could anyone please provide a MR to fix this? I'd be happy to review it then!
@Grevil could you easily fix this and the tests?
I'll merge this now that I'm maintainer here. Thanks!
Yes, please. It's not currently usable this way. Please only add the changes needed to fix the specific issue!
Yes, please. It's not currently usable this way. Please only add the changes needed to fix the specific issue!
What is Blazy version?
Blazy: 3.0.15
Slick: 3.0.6
Still an issue after updating to the latest Blazy and cache clears?
Sadly yes.
Also screenshots of Blazy and Slick UIs and formatter/ Views style forms for exact reproduction in case we are not on the same page.
Instead I'd try to create a minimal reproduction, but currently the days are very busy. I'll come back here asap.
context === document
Good idea, thanks. Probably we should make the context always document if that works all the time. Modern browsers should have no significant issues with DOM traversals.
Hope we got us right here, I thought about letting context pass the once, if context === document
Thanks for your wonderful support and quick reply!
@santanu mondal Remove the unrelated changes please!
@santanu mondal please remove all the unrelated changes!
Thanks for the fix. Test failures are unrelated.
14 days later! 🎉
If I'm too early again, I owe you a drink @avpaderno 😅
Just thought, maybe it could help to always let context === document pass in blazy.once()? Might that help and resolve the issue? (See issue summary for the root cause)
What Drupal version? Fine at D10?
Current version is 11.2.6
Unsure, I can't downgrade, but I think I remember we already had the issue with Drupal 10.
Slick version according to the supported one? See project home for sure, also apply D11 fixes if D11.
Yes, it all works fine if I'm not an admin and if there are no AJAX calls before the document one!
Is it fine without BigPipe?
Yes, just tried that!
Indeed in my case it's a views block (Hero slider at the frontpage) with a slick carousel (vanilla slick) and AJAX enabled on the view.
If you'd like to patch Blazy, try adding:
$blazies->set('use.ajax', TRUE);at the end here:
https://git.drupalcode.org/project/blazy/-/blob/3.0.13/src/BlazyAlter.ph...
Sadly didn't change anything, even after cache clear.
@istryker this still has no stable release, could you please do it? Thank you!