As can be seen in the test failures, the approach in the merge request doesn't work; you cannot simply decode HTML entities when serializing HTML - the result would be both invalid and unsafe.
This was now fixed in drupal 11 and 10, and merge request is the same as my patch at #11, so setting back to RTBC
A command-line check to verify this is working:
SENTRY_ENVIRONMENT=mic-check ./vendor/bin/drush status-report | grep Sentry\ environment
prints:
Sentry environment Info mic-check
@RoSk0 it works for me. Did you verify that $_SERVER['SENTRY_DSN'], etc. are in fact set in your environment?
Are you using the symfony/dotenv or vlucas/phpdotenv package to manage your environment variables, or custom code? If custom code, make sure you are in fact setting $_SERVER['SENTRY_DSN'], etc.
AFAIK, this project already has a phpcs.xml file which instructs phpcs to ignore this error.
Thanks @xjm, I further clarified the comments.
By the way, I noticed that at least one contrib file system decorator (s3fs) still has a dependency on logger. And logging seems even more useful/necessary for s3fs than the default local file_system (all kinds of API errors, network errors, etc.). So I still see some utility in the alternative work-around of a lazy config service that loggers could depend on ( 🐛 Dependency on config storage causes circular reference in service container Needs review )
\o/ thanks I'm in
@nguerinet not trying to "trash" your merge request, but trying to figure out if there's a need for this extra logic in the module, or if you could resolve by using one of the dotenv libraries and/or tweaking your custom code. If it *is* needed, then arguably it would also be needed upstream in the Sentry SDK.
Your steps to reproduce say "Install and enable both modules" so I tried enabling the s3fs modules, but I didn't see any isssues. Maybe configuring and doing something with them is also required?
Could you provide a complete list of steps to reproduce from a clean install of Drupal?
Howabout calling the File Hash duplicateLookup() method directly?
$fid = \Drupal::service('filehash')->duplicateLookup('sha256', $file);
You'll need to replace 'sha256' with whatever hash algorithm you've enabled.
something like this should work?
$file = \Drupal\file\Entity\File::create(['uri' => 'temporary://my-image.png']);
$violations = \Drupal::service('file.validator')->validate($file, ['FileHashDedupe' => []]);
Are you using symfony/dotenv or vlucas/phpdotenv to setup your environment variables? My understanding of both of those packages is that they should populate $_SERVER.
If you're using some other code to deal with environment variables, what is that code? Can you configure/change it to populate $_SERVER, at least for these variables?
This module is basically just a wrapper around the Sentry PHP SDK for easier integration with Drupal. If you weren't using this module, you'd have the same issue with the SDK only checking $_SERVER for environment variables, so I'd prefer not to have a lot of extra code for an edge case where environment variables can't be found there.
It seems you are conflating $_ENV in the title and getenv() in the merge request, but these are actually two distinct mechanisms.
In any case, I'm hesitant to make such a change for a few reasons.
For one, it's a good idea to use one of the PHP packages dedicated to managing environment variables - either symfony/dotenv or vlucas/phpdotenv. These packages can read your environment variables from a file and make them available via $_SERVER, where they would also appear if you were using "real" environment variables (the "recommended" PHP configs actually leave $_ENV empty as a performance micro-optimization).
In certain environments where multiple PHP requests (perhaps from different apps entirely) share the same process, e.g. because of usage of threads, it's not safe to call putenv()/getenv() because the environment variable will be set/retrieved for the entire process, not just the request in question. By default, symfony/dotenv and vlucas/phpdotenv don't use putenv() or getenv(). So, it's generally not a good idea to encourage use of these functions.
The logic to look for environment variables only in $_SERVER actually comes from upstream, in the Sentry PHP SDK. So, it seems I'm not alone in my opinions on this.
So my preference would be that if you're running in to this issue, rather than make a change in this module, you should either use one of the dotenv packages or, if using custom code, set these environment variables via $_SERVER so it's compatible with this module.
But if there's any reason that one of these alternatives would be difficult, I'd like to hear about it.
I'm not sure if the background should be transparent, because what if the background of the webpage is the same color as the artwork? So in the current version, I kept the white color.
We will need to add some copyright metadata to the image, since having it in the git repo will be another avenue of distribution. The creator/author is Hugh D’Andrade and the copyright notice/usage terms are Creative Commons Attribution License (https://creativecommons.org/licenses/by/4.0/).
Ok I added some screenshots for the default out-of-the-box mailer, with and without the patch applied. (The other bug fixed by this patch is that the body is soft-wrapped but words are stuck together due to missing space; I didn't yet dig into how to reproduce it, but I suspect it could happen with a non-default mailer setup.)
@smustgrave I fleshed out part of the steps to reproduce a bit. If we need more, maybe you can clarify what you would like an example of.
SqlContentEntityStorage::save() should be automatically committing the transaction when the transaction goes out of scope.. so if that's not happening, I guess something else already started a transaction?
Filed 🐛 Fix the format=flowed; delsp=yes encoding of email messages Active to fix this on Drupal 11
The flood API provides everything you need to implement this feature - already used by other forms. (If it becomes a distributed attack then you need other mitigations too, captcha etc.)
@dustinleblanc re: laravel env(), getenv()/putenv() can be disabled for thread safety/security: https://github.com/laravel/framework/pull/28740 The library it uses under the hood to deal with environment variables is phpdotenv, which by default doesn't use putenv/getenv but does have a putenv/getenv adapter that can be enabled.
I'd recommend $_SERVER as I mentioned on #11 - it will be filled by default in most scenarios (using real environment variables with the recommended php configs, or using one of the dotenv libraries to set environment variables in default thread safe mode).
Also include a fix for extra spaces in domain list.
Closing for now but feel free to re-open if anyone wants to get spotlight working
@dsnopek I would suggest backporting the actual accepted patch, assuming you find that it works on Drupal 7
@Anybody ok so you're using ubuntu /etc/mime.types, not apache. In that case, looks like webp was added more recently, in January 2023, i.e. in Ubuntu 23.04 - https://salsa.debian.org/debian/media-types/-/commit/6883baf645560bcfe0a... - so it's finally fixed there too
FWIW, I checked the apache httpd source to make sure they do have webp in the default mime.types file, and it was added 13 years ago... https://github.com/apache/httpd/commit/217485c7e1f2f499049b4ff745076e4ed...
Verified that this fix is correct in https://github.com/getsentry/sentry-laravel/issues/894
mfb → created an issue.
So the URL filter, which can be found in the _filter_url() function in filter.module, was developed in the context of previous versions of Drupal, where HTML was serialized by calling saveXML(), thus outputting literal UTF-8 characters, since XML doesn't support most HTML entities. After HTML 5 support was added, HTML serialization results in HTML entities rather than literal UTF-8 characters.
I think the URL filter could be overhauled to support this use case? Parsing HTML with regex is generally frowned upon, but that's what this filter does. If HTML was parsed properly then text nodes could be modified, regardless of HTML entity encoding? Tweaking the serialization rules might be possible too, to partially restore previous behavior, but that seems really hacky.
maybe a comment about how this open-ended
Can you/someone confirm that options are intended to be open-ended (e.g. for contrib module to add/use arbitrary options)? If so, yes, that would be good to document
$_ENV should be fine to use with those dotenv libraries. But, if you are setting environment variables the old-fashioned way, and using the so-called "recommended" PHP configs for development or production, $_ENV will be empty and all the environment variables will be in $_SERVER
tldr: PHP is fun
@mstrelan the issue is that when setting environment variables, e.g. from an .env file, the app may only set them into $_SERVER and $_ENV, not by calling putenv(), so those environment variables will not be available when calling getenv()
@mglaman there's also a problem with $_ENV, which is that the recommended php configs don't enable it (the recommended value for variables order is "GPCS" rather than "EGPCS", i.e. $_ENV is not registered). So, at least in my experience, $_SERVER ends up being the most universal place to read environment variables from.
The tldr explanation is that if PHP is thread-safe, and you call putenv() to set environment variables from a .env file, the environment variables will be set on the whole process, including other threads where they shouldn't be. This problem shouldn't affect server environments where each request has its own process, but in any case, symfony dotenv disables the usePutenv option by default, and to use putenv() with phpdotenv, you have to call Dotenv::createUnsafeImmutable() rather than the usual Dotenv::createImmutable().
I would say environment variables should be read from $_SERVER rather than getenv(), as putenv()/getenv() are generally discouraged due to not being thread-safe
@quietone did you mean this is a duplicate of ✨ Make the map of PSR3 to RFC 5424 log level constants available as a constant Needs work ? I don't think it is a duplicate, as far as I can tell, speaking as someone who has commented on both issues and created one of them. So, if folks are interested in migrating to the PSR-3 log level constants they should probably re-open this issue.
@quietone I actually suggested closing this issue, as it was pretty trivial to copy the map into two contrib modules where this feature could've been useful. Btw, I don't see how that other issue is a duplicate?
Updated the issue summary to explain why this internal method data may be useful to other loggers, not just Drupal core.
@cmlara the PSR-3 logging standard provides a context array to pass arbitrary context data to loggers. So that's what I'm proposing to use (it may be helpful to somehow namespace usage of the context array, however, as I mentioned in the issue summary)
In Raven module → we try to figure out if an error/exception was more-or-less "handled" somehow (i.e. caught, logged and ignored), or more-or-less "unhandled" and logged by one of the final error/exception logging mechanisms.
At the moment, core doesn't add anything to the context array about the disposition of the error or the mechanism logging it, so the best a logger can do is try to figure that out by looking at the backtrace. Which works fine, but is a bit hacky for my taste.
This needs work, as a hook allows other modules to add other form IDs, for which the schema is missing/invalid.
@smustgrave well, as I mentioned in remaining tasks, the $options might in fact be designed to be open-ended? In which case this could just be documented as mixed[]. (If so, should be needs work for that)
@pcambra hopefully this resolves the issue, but please re-open if not
That's great, when it's released can you ping this issue and I'll add a link on the project page
Moving feature request to 6.x branch; perhaps I should also close this as I'm not actively working on it
Moving feature request to 6.x branch
Moving feature request to 6.x branch
I doubt anyone will care specifically about the message variable when looking at a stacktrace, but we could create a new variable for the modified message for "correctness". Can be done on 6.x branch and backported.
Probably can postpone this until the Drupal 11 beta
re: compatible with HTTP/2, I'd hope that proxies convert HTTP/1.1 chunked transfer to HTTP/2 (or HTTP/3) data frames and pass them along, without excessive buffering, but not something I've analyzed
As far as I've seen, Big Pipe does send chunked? All you have to do is print something and then call flush(); and the Transfer-Encoding: chunked header should be added automatically. HTTP/2 or HTTP/3 don't support it though, so you might not literally see a chunked transfer, depending on the stack.