@atowl I think your manual testing proves that my change doesn't break page caching, at least. I'd be curious to know if it affects performance or not. I doubt it does, though. It's less about performance and more about maintainability. I don't know how we could easily test performance. Unfortunately, it doesn't look like the original issue did any performance tests, either.
Also adding that ✨ GPC Implementation Active requires the changes in this task.
@svenryen Since 🐛 DNT (Do Not Track) header detection not working when caching enabled Active won't be fixed, I've created a separate task for the fix required in order for this feature to work ( 📌 Remove custom eu_cookie_compliance_data cache entry Active ). Hopefully this new, separate task will be clearer in what is involved and why it is required.
@atowl I have created a separate task for removing the custom cache entry ( 📌 Remove custom eu_cookie_compliance_data cache entry Active ) and will close this one as "won't fix".
Merge request created.
pianomansam → changed the visibility of the branch 3534024-remove-custom-eucookiecompliancedata to hidden.
pianomansam → created an issue.
@joao.ramos.costa thanks for the ping! I thought that other task sounded familiar. I'll mark this as a duplicate of that resolved one.
Released in v2.3.2
Released in v2.3.2
@lolgm thanks for the tests. With those passing, I've gone ahead and merged this into dev.
@lolgm thanks for the report, fix, and MR. I've merged it into dev.
pianomansam → made their first commit to this issue’s fork.
@atwol yes, you absolutely could. However, I wanted to point out that #3480626 undoes much of the work done in this issue.
@tiage thanks for the feedback on this issue. Feel free to create a separate issue branch and come up with a solution.
Added in release 2.3.1
@facine great work! I made a quick fix to get the tests to pass.
pianomansam → made their first commit to this issue’s fork.
@atowl the merge request on 🐛 DNT (Do Not Track) header detection not working when caching enabled Active does what I propose (removing the custom cache entry). The only line of code that adds the DNT header is the cache context addition.
+ $variables['#cache']['contexts'][] = 'headers:DNT';
A few more responses...
1. On the 'source' side (the SAML message), we get all values as arrays, so there is no way to distinguish between a single value and an array containing a single value. (Feel free to set me straight, if e.g. the attribute data in your SAML responses is somehow marked as "an actual array".)
I'm not sure why that matters, if we can solve for knowing whether or not the destination of the mapping handles arrays.
2. On the 'destination' side, not all fields actually support multi-value. (Case in point: user name, email, language.) And some may be configured/defined to be only single-value, even though the Drupal data model supports multiple.
This is true, but isn't it possible to know if the destination supports multiple values? If it's an entity property (like username, email, language, etc), shouldn't that be accessible from the entity definition? If it's a field on the user, shouldn't that be accessible from the field's configuration?
@roderik thanks for the extensive reply. Unfortunately, `UserFieldsEventSubscriber` has proven to be obtuse and IMHO perhaps overengineered. So I'll most likely be writing a one-off data sync using the SamlauthEvents::USER_SYNC
event for my purposes.
I would also like clarity on why samlauth_user_fields does user linking as well as syncing.
I'm not sure whether you're talking about the boolean map_users config value (which didn't initially exist at all / had its reasons for being introduced, but I'm not sure that's relevant here). Bottom line is: linking is optional.
I'm referencing the SamlauthEvents::USER_LINK
event within UserFieldsEventSubscriber
:
public static function getSubscribedEvents(): array {
$events[SamlauthEvents::USER_LINK][] = ['onUserLink'];
$events[SamlauthEvents::USER_SYNC][] = ['onUserSync'];
return $events;
}
/**
* Tries to link an existing user based on SAML attribute values.
*
* @param \Drupal\samlauth\Event\SamlauthUserLinkEvent $event
* The event being dispatched.
*/
public function onUserLink(SamlauthUserLinkEvent $event) {
pianomansam → created an issue.
@6run0 thank you for the bug report and issue fork with a potential fix. In order for it to be considered, please add tests that demonstrate both the bug and the fix.
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:
- Identify the value of $text
- Write a test that fails
- 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.
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?