This is at least security adjacent, since it's needed to apply symfony security updates..
Well I now cannot reproduce this, after reproducing it a few times. Seems to be some sort of heisenbug?
Ok, sounds reasonable to not worry about the zero size case then (would only happen in edge cases like buggy toolkit, file getting truncated by another process etc.)
filesize() can return zero for empty files; in this case, zero should be returned, not NULL
Realized there is another Request::create() call in AccessAwareRouter, which I also patched
Opened new merge request !10509 for the 10.3.x branch so folks can test/review it. This includes updating symfony/http-foundation, rather than modifying the test to pass on the older version. But if necessary, we could instead remove the update and modify the test.
By the way, this merge request added a try/catch BadRequestException for a $router->match() call.
However, I don't believe the Router::match() method should throw a BadRequestException - according to its upstream phpdoc, it should only throw various routing exceptions.
I have a merge request resolving this over at 💬 symfony/http-foundation Follow up issue for isAdminPath validator Needs work (theoretically we could remove catch statements that shouldn't be needed, but I didn't get to that yet).
Adding another related issue here. I suspect this issue could be closed as a duplicate? If not, please re-open.
Note, some of the related issues do have merge requests that need review, so feel free to review :)
See also the child issue 🐛 Catch potential exception when calling Request::create() in PathBasedBreadcrumbBuilder Needs review which should resolve it (or if not, you have to look at the chained exception stacktraces to see what exception you're hitting)
Actually, looks like Mercury Editor re-implemented the Content Translation page attachments, so you do get hreflang links after all. So, just bug in core Content Translation module I guess.
I looked into this issue, and found that the code in Content Translation module would trigger the same exception. And so, apparently, Mercury Editor decided to simply disable the hook_page_attachments() implementation for Content Translation module, rather than properly fixing the issue. Therefore, you won't have any hreflang tags provided by Content Translation module if you enable Mercury Editor. So, that seems like bugs that need to be fixed in both Content Translation module and Mercury Editor.
In any case, we can resolve the issue in Hreflang module, at least...
@nikolaat do you by chance have steps to reproduce this issue? If so you can fill in the steps to reproduce section of the issue summary.
Hmm, the MR seems to include numerous unrelated changes/fixes? If we decide to go with "Option A" then we would need a cleaner MR (basically just the patch). But in #25 we discussed that we could potentially instead go with "Option B": re-architect hreflang tags to use a lazy builder to add query string to the hreflang links. We don't have a merge request for "Option B" yet.
Seems Router::match() method
I think this can be fixed on all branches by simply updating the symfony/http-foundation requirement to a more recent version that throws the exception, rather than triggering the deprecation? I.e. bump from 6.4.15 to at least 6.4.16?
My workaround in #20 would only be needed if there is a requirement to support older versions of symfony/http-foundation
So it looks like this is specifically for the case where parse_url() returns FALSE.
I was discussing this over in https://www.drupal.org/project/drupal/issues/3489329#comment-15880688 🐛 Drupal core update + update to symfony/http-foundation causes certain views blocks to cause a "client error" and breaks page display. Active
The PHP docs for parse_url() say "This function may not give correct results for relative or invalid URLs, and the results may not even match common behavior of HTTP clients. If URLs from untrusted input need to be parsed, extra validation is required, e.g. by using filter_var() with the FILTER_VALIDATE_URL filter."
So, technically speaking, maybe Request::create() should not rely on parse_url() for parsing relative URLs, or Drupal core should not rely on Request::create() to parse relative URLs/paths.
However, so far I've only noticed parse_url() returning FALSE for "weird" relative URLs (in my case, requests made by a bot), not something that I would have expected to work. So a quick fix could be to test the relative URL with parse_url() and return NULL, before calling Request::create()
🥳
This is why a change record is needed. Basically the same test changes were made when we fixed changed time - now these two fields will finally match.
@smustgrave is there a scenario where a deprecation would be needed here? We didn't need a deprecation for EntityChangedTrait or File created time.
mfb → created an issue.
@smustgrave Can you explain what you mean by BC coverage?
I believe all we need here is a change record (and some reviews to sanity check this :)
This is a bugfix because these methods are already documented to return an integer.
This is analogous to similar bug fixes I made for changed time - 🐛 EntityChangedTrait return type mismatch RTBC - and File created time - 🐛 Fix type hints in FileInterface to align with reality Fixed - where all we needed were change records.
- Also fix RevisionLogEntityTrait::getRevisionCreationTime() to return integers, as it is documented to
- Adjust MediaInterface::getCreatedTime, NodeInterface::getRevisionCreationTime() and RevisionLogInterface::getRevisionCreationTime() to allow NULL return value, as the data model allows NULL values
@praveenpb paths can contain spaces (they get URL-encoded when the path string is rendered to a URL string). But I think trailing spaces would be unlikely because a path seems to be trimmed? If there are some paths that are throwing this exception then they would already be broken - this MR wouldn't make them any more broken.
By the way, I was trying to grok parse_url()'s seemingly arbitrary logic for what returns FALSE, and noticed that the PHP docs say "This function may not give correct results for relative or invalid URLs, and the results may not even match common behavior of HTTP clients. If URLs from untrusted input need to be parsed, extra validation is required, e.g. by using filter_var() with the FILTER_VALIDATE_URL filter."
So, it might not actually be a good idea for Request::create() to rely on parse_url() for parsing relative URLs, or for Drupal core to rely on Request::create() to parse relative URLs/paths, and some followup might be needed related to this? But so far that seems to be a concern only for weird edge cases (a path with a trailing space or a colon and numbers doesn't seem normal) and could take a while to sort out, so we presumably want this fix either way.
@praveenpb for a few reasons.. PathValidator is already catching numerous exceptions and returning FALSE, so it clearly makes sense to do so in a new case. There is some fairly complex logic in Request::create() for deciding which strings are not valid URLs, which also includes the even more complex logic for strings that parse_url() considers "seriously malformed URLs," so you wouldn't want to try to recreate this logic. It's best to leave it to Request::create() (and parse_url()) to decide, and catch the exception that it can now throw.
(It's actually a little bizarre that Request::create() didn't previously throw an exception - I guess you could pass any arbitrary string to it and get a request object.)
I think what's happening is the test expectations needed to be adjusted because this branch is using an older version of symfony/http-foundation that doesn't actually throw. In other words, this MR wouldn't actually need to be backported to this branch until symfony/http-foundation is updated?
I changed the test expectations for it to pass old with the older version of symfony/http-foundation, but on the other hand, if this branch is still supported, then actually symfony/http-foundation should be updated instead and we should revert the last commit?
mfb → created an issue.
Interesting! In that case I'd want to look at those problems, but I don't know specifically where you were seeing that.
I can only talk about the two brand new issues I've looked at related to the new Request::create() exception, where it totally makes sense (imho) to catch it where it's thrown because we can logically return FALSE or NULL - https://git.drupalcode.org/project/drupal/-/merge_requests/10306 and https://git.drupalcode.org/project/drupal/-/merge_requests/10307
If you don't accept https://git.drupalcode.org/project/drupal/-/merge_requests/10306 for this issue then I can open a child issue (and I realize it needs a test, but I haven't gotten there yet :)
Are you saying you were seeing an infinite loop prior to the recent symfony update? For me that's not the case, this was a new bug report.
@casey re: the infinite loop + out of memory, I also needed to catch the new Request::create() exceptions in a different place, see https://git.drupalcode.org/project/drupal/-/merge_requests/10306
Well, the root of the issue here is that Request::create() never threw exceptions previously, but now it may. So, dealing with these exceptions did seem like the correct fix to me - and resolves my infinite loop.
Of course there is more than one Request::create() call in core, but there actually are now that many, outside of tests
Ok great. I added a test which passes a new line character to PathValidator - I don't know specifically what caused the exception in your case, but I don't think it matters, we just want to ensure it catches a BadRequestException and returns FALSE.
I looked at how some invalid paths are handled with MR !10307 applied, and it appears they are handled correctly, because the invalid path is actually "repaired" by being URL-encoded. Some examples:
Drupal\Core\Url::fromUri('internal:/1:1')->toString();
// "/1%3A1"
Drupal\Core\Url::fromUri('internal:/foo ')->toString();
// "/foo%20"
Tagging needs followup because I'm not sure work here is complete. The Drupal\Core\Url methods are not actually documented to return a BadRequestException. Either this exception shouldn't be thrown, or should be caught and re-thrown as a different exception, or should be documented.
@alfthecat can you check if MR !10307 resolves your issue? If not, then I guess a full stacktrace of the BadRequestHttpException would be helpful, I was really just guessing/assuming that this was the Request::create() call throwing the exception.
I'm working on an alternate fix for this: Catch the exception when calling Request::create() in core/modules/system/src/PathBasedBreadcrumbBuilder.php
Adding a remaining tasks item to analyze PathValidator class. On the one hand, it can be safer to throw exceptions for security reasons, as Request::create() now does in its most recent versions. On the other hand, PathValidator is a validator which returns FALSE if the path is not valid - thus it may be expected to catch this new exception and return FALSE.
@alfthecat the most obvious cause of this, looking at your view, would be if the product_id has a trailing space or control character. The view creates links with path: /product/{{ product_id }} and this path will be processed by Drupal\Core\Path\PathValidator, which calls Request::create('/' . $path); , which now throws that exception.
Huh I couldn't reproduce the test failure on this MR? 🙃🤷
oh somehow missed that? nevermind :p
This issue ended up in the release notes for Drupal 11.0.9 but presumably that's a typo, since it was reverted
@liam morland I was referring to how we need to look at the data model. For example, Media and Workspace entity, like File entity, allow a NULL created time to be stored in the database. So we need to allow the method to return NULL in these cases, and those interfaces need to be adjusted to match.
@liam morland File entity allows NULL created time, and thus databases may actually contain NULL for that column. That's why I changed FileInterface to return int|null.
I believe most entities have a NOT NULL created column, but it's worth double checking them.
By the way, for comment entities, created time cannot *really* be null, as that column has a NOT NULL specification, so you cannot save a comment with a null created time; when you create a new comment entity, the created time is automatically initialized to the current time. So I'm not sure why or in what circumstances this method would return null, aside from someone forcibly setting the created time to null..
Looks good to me.
When we made this fix for File entity - https://www.drupal.org/node/3262371 → - I assumed that adding type declarations would need to happen in a major version, so I didn't work on it in that issue.
Tagging "needs followup" since there are other cases of getCreatedTime() and getRevisionCreationTime() that need to be fixed
Drupal 11 is already supported
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