Drupal 11 compatible release is already available
Yeah only bad code interpreting the feed could be a danger here, because untrusted third party strings need to be just that, untrusted, and run thru something like HTML Purifier or the filter built into drupal core; i.e. security issues shouldn't be possible regardless of what's in a third party string.
Closing this as no one has worked on it in a while. Feel free to re-open if someone wants to contribute a merge request.
Closing this since no one worked on it. Happy to review merge requests but this didn't have one. Feel free to re-open if someone wants to contribute :)
Closing this because that project was discontinued
Already released for Drupal 11
This module already works on Drupal 11
Ok great, I'll roll a new release soon.
It would also be nice to fix the underlying bug in BigPipe module, but I haven't figured that out yet. At the very least I should be able to write a failing test..
@joel_guesclin Good news, I think I was able to work around the bug in core - please try the merge request version of piwik_noscript, or apply the patch.
I did some refactoring to use automatically-created placeholders, rather than manually-created placeholders - this bypasses the bug in BigPipe module. The automatically-created placeholder is a bit silly, because it means we are executing javascript to place a noscript element, but this code path allows BigPipe to apply drupalSettings correctly.
@catch I did some more debugging on this and determined that this bug only affects manually created, non-markup placeholders, which are essentially skipped - although still broken - by BigPipe.
Many placeholders will not be affected by this bug because they use automatically-created placeholders ('#create_placeholder' = TRUE)
Ok, so we don't yet know why your drupalSettings is getting loaded in the <head> as opposed to in the <body> - it wouldn't be the "solo" theme since that doesn't have any libraries that specify both header: true and dependencies: core/drupalSettings
You would probably need to give me all your installed modules (contrib and custom) so that I could try to reproduce it - or figure out on your own why drupalSettings is getting loaded in <head>
@joel_guesclin if you're able to track down a contrib module you're using with a library that specifies header: true and depends on core/drupalSettings, that would seem to be what's triggering this issue - at least in my testing.
AFAICT, the AJAX system seems to be able to merge new settings from an AJAX response into the global settings just fine, it's just not happening when BigPipe processes placeholders.
A comment in Drupal\bigpipe\Render\BigPipe says
If additional asset libraries or drupalSettings were attached by any of the placeholders, then we need to re-render scripts_bottom.
- seemingly forgetting that we don't necessarily know where drupalSettings lives, maybe it was in the <head> element. Fingers crossed someone more knowledgeable about the AJAX system could figure out a good way to resolve this.. :)
Since this seems to be a bug / limitation in Drupal core, I haven't yet figured out a way to get things working with drupalSettings being loaded in the <head> and BigPipe enabled.
At the very least we could fail silently - just do not send the beacon if drupalSettings.piwikNoscript is missing. This has the disadvantage, however, of not notifying site admins about the broken functionality :/
Filed an issue for the core bug I seem to have found: 🐛 Placeholders cannot attach drupalSettings if drupalSettings was loaded in header Active
Well, I was able to hack together a custom module that might be reproducing this issue
I suspect this might be caused by drupalSettings being loaded in the <head> element, as you mentioned in #8.
Do you have a contrib or custom module with a library that specifies header: true and depends on core/drupalSettings?
Also, could you try disabling BigPipe module - this might resolve that particular issue.
Well that's pretty strange that piwikNoscript is completely missing from drupalSettings, as it's explicitly added by code in piwik_noscript.module
Re: your "steps to reproduce," are you able to reproduce this bug on a clean install of Drupal 10.4? I wasn't able to.
In order to resolve this I would need steps to reproduce the issue from a clean install of Drupal, such as install this theme or install that module. I did try installing "solo" theme from drupal.org but piwikNoscript was still showing up in drupalSettings.
@joel_guesclin I still have no idea what could cause this, but I created a merge request that you could try applying to see if it resolves the error. Basically we could defer firing off the beacon until the document is loaded - hopefully by then the drupalSettings are properly available?
@joel_guesclin you confirmed that you have basically the same error with javascript aggregation disabled, but you didn't confirm what you see in the page source when the error occurs. Do you see the markup I described in #2 or are they in a different order, or is something missing?
@joel_guesclin the error method suggests that your theme is somehow executing the javascript before drupalSettings have been defined?
The javascript for this module defines core/drupalSettings as one of its dependencies, so I would have thought that this shouldn't happen..
i.e. with javascript aggregation disabled, you should see <script type="application/json" data-drupal-selector="drupal-settings-json"> which contains something like "piwikNoscript":{"url":"https:\/\/example.test... and then further down <script src="/core/misc/drupalSettingsLoader.js"></script> and then still further down <script src="/modules/contrib/piwik_noscript/piwik_noscript.js?v=1.13.0"></script>
What's the policy for security updates of dependencies on security-only branches? Assuming it's ok and/or required to do so, the composer update could be applied on that branch
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