One minor issue I noticed is excessive logging - three messages are logged for a bad request: "page not found" warning, "client error" warning, "php" error
The 8.x-1.x branch is already compatiable with drupal 11
Actually, there is no hook_seckit_options_alter() in 7.x-2.x, so no need for a new release on that ancient branch
Looks like this is handled differently between the Sentry PHP SDK 1.x and Sentry PHP SDK 3.x branches, so this will need to be applied only to the 7.x-3.x and 7.x-2.x branches (not 7.x-4.x)
This would be wonderful for documenting array shapes for function parameters as well. Those have some additional phpcs errors:
- Parameter $options is not described in comment (Drupal.Commenting.FunctionComment.ParamMissingDefinition)
- Missing parameter name (Drupal.Commenting.FunctionComment.MissingParamName)
- Parameter comment indentation must be 3 spaces, found 1 spaces (Drupal.Commenting.FunctionComment.ParamCommentIndentation)
@smustgrave I tried to make it a multiline array shape definition, which works fine with phpstan, but apparently Drupal\Sniffs\Commenting\FunctionCommentSniff (part of the drupal/coder project) cannot parse such phpdoc properly, see 🐛 Array shapes in multiple lines are not supported. Active , so I guess we should postpone this
You will have to explicity tell the php:8.3-apache docker container to pass an environment variable into $_SERVER
Create or mount a file in the docker container: /etc/apache2/conf-enabled/env.conf
This file should read PassEnv SENTRY_ENVIRONMENT
Restart and the SENTRY_ENVIRONMENT environment variable should now be available in $_SERVER
As I mentioned in #9, silently ignoring the null rather than logging a warning seems fine to me.
Setting to fixed as I don't think there's more to do here.
Well, we already allow the dedupe validator to be added to form fields on demand, i.e. explicitly add an attribute to the form element that says "Enable file deduplication"
The site-wide dedupe setting, on the other hand, doesn't have anything to do with forms. It's a file validator for all new files.
So I'd recommend not enabling that setting if you don't want it always on. Use the form-field specific dedupe validation instead.
But, if you insist on hacking that setting to be disable-able, I would say your custom module should add the event subscriber to all requests (not just via form alter), and disable the dedupe constraint based on some aspect of the file entity (such as a unique path, or whatever else might be available).
It would also seem reasonable for core to start adding more context to file validation events, so that custom event subscribers have more to work with, but I didn't look into if/how that would be possible.
Here is some code that might work for you in a hook_form_alter():
$listener = function ($event) {
foreach ($event->violations as $key => $value) {
if ($value->getConstraint() instanceof Drupal\filehash\Plugin\Validation\Constraint\FileHashDedupe) {
$event->violations->remove($key);
}
}
};
Drupal::service('event_dispatcher')->addListener(Drupal\file\Validation\FileValidationEvent::class, $listener, -1); (Let me know if this does or doesn't work for your issue)
@infojunkie: Would you be able to either share a module that reproduces this issue, or write a failing test for this case?
@quietone this is not actually a duplicate: 🐛 EntityChangedTrait return type mismatch RTBC only addressed changed time returning a string. We still need to address created time returning a string for the various core entities.
@rp7 Interesting, I guess phpstan doesn't warn about such issues in phpdoc? These need to be migrated to actual type declarations at some point..
Globaly when using docker with environnement variable with php official image for example, all variables mentions in .env files are not available in your $_SERVER but in $_ENV.
Thanks @nguerinet that's good to know. I'm still fuzzy on when/how variables are missing from $_SERVER, e.g. if I make an .env file and run docker run --env-file ./env php:8.3-cli -r "echo \$_SERVER['SENTRY_ENVIRONMENT'];" the SENTRY_ENVIRONMENT variable is printed.
@kim.pepper I don't understand why you say "triggering hooks during container build" - the hook was triggered when the service was created, i.e. when a request needs to use it for the first time, not when the container is built.
I was timing the alter hook on requests where the service is created (e.g. because guessMimeType() was called at least once). Since it's when the service is created, the 10ms slowdown only happens at most once per request.
alterMapping is called whenever the file.mime_type.mapper service is created (for example when ExtensionMimeTypeGuesser is constructed). So the alter hook fires on any request that needs to create that service.
E.g. if a page renders an entity that displays an audio file field, the service is needed and the alter hook fires. But once that entity is cached in the render cache, it will no longer render, and therefore the service won't be created, so the alter hook won't fire. Maybe that is the caching you were referring to, I'm not sure? There are also admin pages where the service is created, and the alter hook fires, without any sort of cache being involved (e.g. managing display of an entity with a file field).
Projects like https://www.drupal.org/project/sophron → that use the https://github.com/FileEye/MimeMap library have a different data structure.
I looked at Sophron module, and it has a totally different mechanism for allowing other modules to alter its mapping - it dispatches an event. I guess you are saying that a future version of Sophron module could start invoking this alter hook?
@kim.pepper no, there is nothing caching the map, whether in the container or otherwise. Both the old and new alter hooks run whenever the map is used.
Realized I was being lazy and I should benchmark it. So, I found that implementing the new hook in File MIME module (configured to add all the MIME types from the /etc/mime.types file) adds about 10 milliseconds of execution time. This is probably fine for those cases where the hook is invoked during file uploads - such requests are going to be extremely slow regardless - but not ideal for other pages where the hook might be invoked (e.g. managing display of an entity with a file field). Admittedly, File MIME module doesn't have very many installs (and more installs of the Drupal 7 branch than the current branch).
Yes I see know it's possible to call addMapping(). But I'd have to iterate through and make hundreds of method calls (if someone configures File MIME module to parse their /etc/mime.types file), with an array_search() in each one, so it just seems slower, that's all. And I'd have to double check there is no behavior change from how it worked previously.
Well I'm looking at how to still allow File MIME module → work without using the deprecated hook_file_mimetype_mapping_alter(). It looks like the new alter hook doesn't allow contrib module to get the mapping so it can make various changes, and then apply it via setMapping().
And different data structures? Well that would make it even more difficult for a contrib module to alter the mapping, lol sigh. I confess to not actually following this issue.
The 8.x-1.x branch already works with 11.x
Why not allow contrib modules to call getMapping() so they can arbitrarily alter the mapping?
This is a bit strange, as this code is more-or-less taken from core Content Translation module. So, you'd think you would also see this error when Content Translation is in use.
However, I can look into trying to reproduce this...
The 8.x-1.x branch already supports 11.x
The 2.x branch already works on 11.x
The 2.x branch works with 11.x branch
8.x-1.x branch should work fine on 11.x already
👍 yes this is correct as far as I know - I use extension_loaded('apcu') && apcu_enabled() in a contrib module, and we're way beyond APCu 4.x, as 5.x is required for PHP 7 (much less PHP 8)
@rodrigoaguilera thanks for looking into it! Did you notice that core itself uses some additional undocumented option keys? See the remaining tasks section of the issue summary.
...and it looks like there is already a third-party tool pretty similar to what I mentioned - the fileeye-mimemap update utility retrieves MIME types from both Apache and freedesktop.org, merges them, and generates a map. This library could be incorporated as a runtime third-party dependency, or at least be leveraged to help maintain the existing mapping.
@kim.pepper But in this case, we have a "fake" file: 'fakedFile.' . $extension So, we only want to retrieve a MIME type based on the extension, not anything else that file.mime_type.guesser might use. Thus only file.mime_type.guesser.extension is used directly.
We could create a followup issue to do something like.. create a script to audit/compare drupal's mapping vs. the MIME type databases typically found in operating system distributions? There are actually at least two such databases: The /etc/mime.types file and the /usr/share/mime directory.
Oops
oof :( Good catch, thanks!
Added steps to reproduce to the issue summary.
I was thinking of this from the perspective of just missing data, like if a language was missing from the array of languages in LanguageManager.php, as Drupal maintains a mapping of known file extensions and MIME types, and file extensions can be configured manually. (Of course neither languages nor file extensions/MIME types are an exhaustive list, as they are only added when someone notices them missing..)
@smustgrave What did you have in mind? You wouldn't want to add a test for each of the ~369 mappings. It's just a couple big flat arrays.
@smustgrave There is already test coverage of the extension to MIME type mapping, so I don't think additional test coverage is needed here.
Hiding out of date patch
Closing for now but feel free to re-open if anyone wants to suggest documentation improvements
Well, in that case, at the very least, a developer should be able to implement their own File Hash validation in a custom module, which they can enable or disable as required.
The validation feature built-in to File Hash module is, in my humble opinion, a proof-of-concept for developers (as mentioned on the project page → ), since a real-world site will probably need a better user experience and more features (for example, automatically re-using the existing file rather than simply rejecting the file upload).
Already resolved
Already resolved
Already resolved
Already resolved
This was resolved on the 3.x branch
This was resolved in various other issues.
New features won't be added to the 2.x branch - can you try this on the 3.x branch?
The 3.x has a new API - filehash_file_validate() no longer exists.
You should be able to add/remove the FileHashDedupe validator from a form's array of upload validators.
Not sure if i will have time (as I'm not using this module at the moment) to help, but I'll go ahead and tag this for future reference
Didn't actually test it to confirm, but LGTM (might be nice to add some test coverage for the config form)
Yes I guess this was a long-standing bug, that segments weren't taken into account. I'm updating the issue summary to also mention this bug.
I don't see a problem in practice with displaying ByteSizeMarkup::create($memory_info['seg_size'] * $memory_info['num_seg']) to the end user, as it's generally going to be the same as displaying ByteSizeMarkup::create(Bytes::toNumber(ini_get('apc.shm_size'))). The difference between configured size and reported size should be tiny enough that ByteSizeMarkup doesn't care.
Either way works for me, really. So if folks are adamant about displaying ByteSizeMarkup::create(Bytes::toNumber(ini_get('apc.shm_size'))), sounds good to me.
Back to needs review
Alternate resolution which seems less verbose
Looks like various packages have the suffix as "-flat-xml" rather than "+xml", so I'd go with that as my best guess
@annmarysruthy the array of file extensions should be in order by the numeric value.
We're going to need to add the other extensions too, but idk what they are supposed to be - maybe same as their od* equivalent, but add a "+xml" on the end because it's XML? Just a wild guess. If we can't find the mapping specified/documented somewhere, we could copy from some other package that maintains a mapping.
@blb I created a followup issue @ 🐛 Some file extensions in field.field.media.document.field_media_document.yml are missing from ExtensionMimeTypeGuesser Active
mfb → created an issue.
Yeah I meant flush the entries that can be flushed, pertaining to a discrete test run that is over, not flushing everything. But idk how that'd be done, since a testrunner is a cli process that doesn't have access to APCu..
@smustgrave yes the test-only will fail only on environments with the default php config, not the CI environment.
Yes I can see how the APCu cache will fill up as tests are run (literally, I can see this happening locally). Although it might make sense to flush it somehow, but I guess that's easier said than done? Anyways, I added a functional test.
It looks like the CI environment sets the APCu size to 3GB - whoa, that seems like a huge waste of memory, or could tests actually use that much APCu cache somehow? So, I don't think it will be easy to reproduce this exact bug there, unless there's a way for tests to modify the php config files, but we could add a test that verifies this case in test environments using the default config.
Clarify the proposed resolutions.
Added this scenario to the existing functional test
Looks like MimeTypeGuesser::guessMimeType() returns 'application/octet-stream' if all of the registered guesser(s) have returned null. But ExtensionMimeTypeGuesser::guessMimeType(), being a specific guesser, returns null.
In this case, the specific guesser is being called directly, which is a bit odd, but doing it this way means a null mime type is possible.
So yes, just handling this possible null seems like the easiest solution.
@smustgrave the premise is configuring an unknown file extension, so I think it's reasonable for the mime type to be null.
And silently ignoring/masking the null seems fine to me, unless folks think it's important enough to log a warning that an extension with unknown mime type was configured.
Left at needs work since I suggested changes on the merge request.
@Chandansha Oops I forgot to mention configuring the field formatter in the steps to reproduce, that is probably what you were missing
Great, thanks! To be honest, I have no idea why this line is even there, I don't believe it's necessary? So I pushed a commit to remove it. Please re-open if this isn't the proper resolution
I couldn't yet reproduce this, but feel free to re-open if you can provide more info
Documenting how environment variables work feels a bit out of scope, since it changes so much depending on the environment, but if someone wants to contribute a documentation merge request I can look at it.
Closing since I don't have a clear explanation for why we also need to look for environment variables in $_ENV
But there may very well be a good reason (common use case, etc.), so feel free to re-open this issue if providing more info.
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.