🇺🇸United States @pianomansam

We can't use render cache, but we might be able to just put everything into a single regular cache entry and fetch from that instead of having to build it over and over again on every page?

Why can't we use render cache? The three existing parts of the $cid (language ID, domain ID, theme name) are already covered by the render cache. In fact, having to add info from another module, the domain module, is a code smell that perhaps this isn't the best approach. The domain module's documentation suggests users add the url.site cache context so that the cache can vary based on the domain. So the EU Cookie Compliance module shouldn't need to worry about the domain. However, because it micromanages this part of the renderable content, it needs to account for all the ways it may vary.

In working on the 🐛 DNT (Do Not Track) header detection not working when caching enabled Active bug, I traced the issue to the custom cache entry's $cid not varying based on the DNT header. If a custom cache entry was not deployed, that bug would easily be fixed with one line of code adding the DNT header to the cache context. Instead, we need yet another variance of the $cid.

So to summarize, why can't we let the render cache do its job? Micromanaging a separate cache entry causes additional complexity, makes it easier to cause bugs (DNT header issue), and causes this module to care about unrelated modules (domain module).

@svenryen In summary, I'm questioning the work done on #3015612: [1.x][2.0.x] Better caching for cookie performance popup → and proposing that we undo it. I do not understand why that task says we cannot use the render cache. I will move my discussion to that task.

Drupal 10.1+ introduces a different configuration → for aggregates, and thus S3FS no longer ✨ Add support for s3fs to use the assets:// stream wrapper Postponed sends aggregates to S3. There is a separate s3fs_assets → contrib module that promises to do that, but it only has a dev release.

Remove mention that  SAML Authentication →  attribute sync is a D8 wishlist item, since it appears to be complete.

Setting this to active since no work has been done yet.

@chucksimply I'm unable to reproduce this. I've tried to make this happen with a multi-value image media field and displaying the image style url of the second entity in the field, then falling back to the first entity in the field. The functionality worked as expected.

[node:field_banner:1:entity:field_media_image:featured_image_desktop:url|node:field_banner:0:entity:field_media_image:featured_image_desktop:url]

Can you reproduce from a clean site install and provide all the necessary steps? Or even better, create a feature branch and write a test reproducing the issue?

Could this issue be related to the token’s depth? Even using just
[easy_email:field_report:entity:field_photos]
isn’t recognized as empty, and the process stops.

@chucksimply Sorry to hear that you are experiencing issues. When you uninstall token_or, does this token produce a value?

@ok4p1 thanks for the bug report and the patch file. In order for this to be considered, please open a Merge Request with your changes. Also, please write a test showing that it solves the issue.

MR updated to latest dev

@atowl regarding the domain cache context, the domain module documentation suggests adding url.site to the services file. So users of that module should follow its directions, as doing so will resolve all sorts of things. If domain module users are experiencing issues with this module, it's not due to this module but rather not properly configuring their site for the domain module.

I realize this will be fixed in the other issue, but I wanted to upload a 6.x compatible patch file.

I agree with the idea of releasing a stable version of what's there right now in 3.x and then starting 4.x to start implementing oauth2-client.

@jcnventura regarding finding another maintainer, this project already has five! Has it not really been maintained?

I was wondering this, too. The 8.x-1.x releases are marked as recommended, yet they have many fewer features, fewer fixes, and half as many reported installs (6k vs 12k).

pianomansam → created an issue.

@lhridley that was a mistake. I've updated the related issue to what I believe I meant to link to.

@ruslan piskarov the new 2.3.0 release adds D11 support.

@ruslan piskarov Drupal 10.x has been supported since v2.1.0

@webflo, that's completely understandable. I will be moving off Drupal 9 and PHP 7 as soon I can. In the meantime, I'm just trying to leave a breadcrumb trail for anyone else who may come along and experience the same issue. Would it be possible to document the decision from this issue and #3481514 on the module's home page? I realize it's too late to modify the composer.json in 2.0.0-beta2 to enforce PHP 8, but mentioning the requirement on the home page would be very helpful.

Updating this issue's meta to critical because this 2.0.6 is completely broken.

This module has a release for Drupal 9, though. If this module doesn't want to support Drupal 9, it should have a separate release that doesn't support Drupal 9.

I've opened a MR that makes the canonical substitution language neutral, by default. I don't know if this will work in all circumstances, but it works for session language detection.

The OP said:

I have reproduced this with both latest 8.x-5.x and 6.x releases.

So I'm reopening this and labeling it for 6.x. I am presently experiencing this issue on 6.1.5 so I'll be posting a patch shortly.

FWIW the patch in #2 does NOT catch all the stray commas that make this module incompatible with PHP 7.x

starting from Drupal 9.1.0 you can use PHP 8.0

Yes, but Drupal 9 is compatible with PHP >7.3 so this module should be as well, OR it should explicitly call out the PHP 8.0 requirement in its composer.json file and d.og module page. And if the incompatibility is simply due to an extra comma as the patch in #2 suggests, I highly recommend just fixing that so it can support a wider installation base.

Since this issue requires the fix in 🐛 DNT (Do Not Track) header detection not working when caching enabled Active , here is a version of this module's MR patch file that assumes 3480626 has been applied first. You can commit it to your code base and update your `drupal/eu_cookie_compliance` entry in composer.json to something like this:

"drupal/eu_cookie_compliance": {
    "#3480626 - DNT header detection not working": "https://git.drupalcode.org/project/eu-cookie-compliance/-/merge_requests/150.diff",
    "#3400695 - GPC Implementation": "patches/eu_cookie_compliance_gpc.patch"
}

I've opened an MR that addresses this issue. However, please note that my MR won't work until 🐛 DNT (Do Not Track) header detection not working when caching enabled Active is fixed. The GPC header detection will be equally broken as the DNT header detection due to not varying the cache based on headers.

pianomansam → changed the visibility of the branch 3400695-gpc-implementation to hidden.

pianomansam → changed the visibility of the branch 3400695-gpc-implementation to active.

I'm going to switch this back to 1.x since I have a solution for that branch.

pianomansam → changed the visibility of the branch 3400695-gpc-implementation to hidden.

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

I have opened a MR that removes the custom caching of eu_cookie_compliance_build_data() and adds the DNT header to the cache context of eu_cookie_compliance_page_attachments(). Default services (default.services.yml) already has the language and theme cache contexts. And the Domain module suggests adding `url.site` to that list.

I did not see any existing tests for this module, and even if I did, testing this seems pretty difficult. So I'll simply mark this for needing review and let others test it out.

pianomansam → created an issue.

Fix released as part of 2.2.1

With tests added and passing, this has been merged into 2.x-dev

Triggering tests

@lolgm thank you for the report and the in-depth research. The next steps are to write a test proving the issue, update the code to fix the issue, and write a test showing it is fixed.

@captain hindsight Always open to pull requests!

@bryanmanalo that's unfortunate. Does your patch resolve this issue for your site? If so, we'll need to determine what kind of value is being pass in as $text. I see your patch uses empty($text) while the current release uses !$empty so there must be something that empty catches that false does not. So, in order to get this resolved we need to do the following:

1. Identify the value of $text
2. Write a test that fails
3. Fix the logic so that the test passes

@bryanmanalo this issue was resolved in release 2.1.1. Are you still experiencing it?

I'm also experiencing this issue.

Patch in #5 fixes this issue for me as well.

Because this module includes a submodule for Webform, we will need to wait until Webform has a Drupal 11 release.

Updating to postponed as more information is needed from the OP to resolve this.

@heliogabal I'm sorry to hear the module isn't working for you. The scenario you describe sounds pretty complicated and won't be easy to replicate. Can you provide a simple scenario where you aren't seeing it work? Perhaps the module does work in simpler scenarios but not complex ones. We'll need to narrow down exactly where it breaks down.

Drupal 10 has, yes. But this module supports Drupal 9, which is had ES5 as its standard. If a module is to support Drupal 9, I think it should also support all the browsers that Drupal 9 supported.

https://www.drupal.org/node/3305487 →

I was experiencing this issue when using the 2.x branch. When putting a console log on the JS click handler, the console output my message 16 times for a single click. When I looked closer at the code, I realized the JS reapplies the click handler each time Drupal does a JS attach. Which in my case, was 16 times. I've created an MR that uses both the core/once library as well as Drupal attach's context to make the click handler only occur once.

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

Changing the status back to needs work as there are unresolved questions on the merge request. Further, the merge request doesn't do anything to solve the issue raised in #23. The fix in #23 is still a manual fix that should be taken care of programmatically with a migration/update.

@jayhuskins thank you for the bug report and initial MR. For it to be accepted, please create a failing test demonstrating the issue and a passing test.

@i-trokhanenko that's awesome. Thanks so much! With this out of the way, it would fantastic to see progress on ✨ Make compatible with simple_sitemap 4.0 & domain_entity Fixed

Can one of the 6 maintainers look at this? Drupal 9 is now unsupported. What needs to happen to get this across the finish line?

pianomansam → changed the visibility of the branch 3379202-drupal-10-compatibility to active.

pianomansam → changed the visibility of the branch 3379202-drupal-10-compatibility to hidden.

This was included in ✨ Support for webform Needs work

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

I got tests to work, so I expanded them to make sure they handled both current-user and current-page tokens. Sure enough, they caught an issue with current-user tokens. That is now resolved and I'm content with the amount of test coverage, so I'm merging this into dev.

@JeroenT any chance you can help get Gitlab CI working on ✨ Support for webform Needs work ?

@jmaties While I'm trying to get the testing rig working with the submodule, it looks like TokenOrWebformFunctionalTestBase is throwing a deprecation error in Drupal 9:

Calling Drupal\Tests\WebAssert::elementTextContains with more than three arguments is deprecated in drupal:9.1.0 and will throw an \InvalidArgumentException in drupal:10.0.0. See https://www.drupal.org/node/3162537 →
1x in TokenOrWebformFunctionalTestBase::testGetFirstParam from Drupal\Tests\token_or_webform\Functional
1x in TokenOrWebformFunctionalTestBase::testNotGetParam from Drupal\Tests\token_or_webform\Functional

While I see tests for the submodule, it doesn't look like they are being run by the continuous integration.

@jmaties thanks for your work on this. It looks like this is still in "Needs work" but I wanted to give some quick feedback.

First, it looks like you have duplicated the entire replace method. It would be great if this was more surgical, only making the changes necessary for this functionality to work, and then handing it off to the parent. The reason for this is that the underlying replace method in WebformTokenManager could change at any time. If/when it does, those changes won't be reflected in our version. Thus increasing maintenance and the chance something could break.

Second, to be accepted, we'll need tests added to your MR. Initially, we'll need tests that fail and demonstrate the issue. Then we'll need passing tests that demonstrate it is resolved.

This issue appears to be fixed in 8.x-1.3

Yes, that appears so. Closing.

Specifically, it's not clear to me how to do this step:

You will also need to call this module's AnonymousCsrfTokenGenerator service that wraps the CsrfTokenGenerator from core in order to complete the implementation

I too am struggling with getting this module working. Perhaps a small example module could be added to the project?

If #2274177: Allow altering settings per editor and field → will fix this, better to make this a duplicate and track the other issue, because at this point in time, the issue is not fixed.

@darrenwh please read Smart IP page under the "Configuration / Cache Performance Note".

Am I blind, or does that section no longer exist? I, too, would like to know how to get this running behind Varnish or nginx

I was wondering if it was the token filter module. What's the order of filters? If the token filter first?

@facine Hmm, can you provide a bit more context? Is a text format involved? Any particular modules or contexts where this is happening?

@facine, thanks for the updated patch. I'll review it.

Also, thanks for the scenario where this could be an issue. Is this particular one breaking something for you? Whether you use <a href="mailto:[current-user:mail|'email@example.com']"> or <a href="mailto:[current-user:mail|"email@example.com"]">, the result will be the same:

<a href="mailto:email@example.com">

@facine can you also provide an example of how this affects you? The token replacement will also replace the double quotes, so it's unclear to me how this is an actual issue.

@facine I think adding this feature is great. As it is currently written, though, it looks like there's nothing that forces the content to use the same type of quote character; a double quote could be first and a single quote second.

pianomansam → created an issue.

I've created an issue fork and pull request that does what I propose to do. It only uses the optimizer if CSS preprocessing is enabled.

pianomansam → created an issue.

I was able to reproduce this issue. Apparently, some modules directly send NULL into the token service.

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

@fromme testing with the quiz module sounds pretty involved, so I'm not sure it will work here. Out of curiosity, what's the token you're using that gives the error?

@fromme thank you for your report and patch. I'm having a hard time reproducing this. Before a patch can be accepted, we need a test that can reproduce and test that the patch fixes the issue. Can you at least help me reproduce it?

Patch is also working for me. A release should be created.

The code is now available in git. Closing.

Updated with null coalesce

Created a patch to check that $credentials is an array before returning it.

pianomansam → created an issue.

Thanks!

I've created an issue fork and merge request to resolve this. I've tested this on both page and block view displays.

Attaching possibly related issue ✨ Should refresh work on the view block displays? Active