I discovered an API method we can use to convert the description markup to plain text: PlainTextOutput::renderFromHtml(). And for some reason, the default title is empty string, but the default description is null. So, we need to handle that potential null by only using the description if it's present.
Opened up a MR for review. Since title is apparently used in de-duplicating items, i thought it might be a good idea to save the truncated description instead of an empty title? But also, since a NULL or empty title would be valid (if saved programmatically, for example), I also patched the label() to use the truncated description, if necessary. Since the description can be HTML-encoded markup, we have to strip tags and HTML-decode the description before using it as the title, otherwise the markup would be displayed to the end user.
Setting to needs work because this isn't really fixed by the patch, yet :)
I updated the issue summary with a pretty-important remaining task: Figure out exactly how to handle RSS items that are missing a title or have an empty title, due to the item title being used in the UI, e.g. when listing items, and by the label() method; all this stuff still needs to work for items without a title!
Far as I can tell you're good here, the dependency is on ^2.17, which allows any later 2.* version to be installed, including the current version, which is 2.24.0
Tests are failing, but I don't think it's related to this MR, as tests are also failing on the 2.x-dev branch
So I think this could be RTBC?
Added a commit to resolve a deprecation notice I've been hitting on a production site:
Deprecated function: Drupal\aggregator\ZfExtensionManagerSfContainer::setContainer(): Implicitly marking parameter $container as nullable is deprecated, the explicit nullable type must be used instead
As far as sustainability a few things come to mind:
- A file hash submodule could report duplicate files and even allow them to be merged.
- A "sustainability" contrib module could instruct site admins to install helpful modules and configure them more sustainably.
I don't intend to enable the dedupe validator by default. It is a complex feature that could be enabled site-wide, on a per-field basis, or programmatically; there are also the choices of "strict" dedupe and "original" dedupe. And I do consider it a "proof of concept" - sites may want to improve the dedupe UX.
@liam morland did you see my comments in #30? I feel like it would be helpful if this issue could decide to either trigger a deprecation if null is returned in cases where it is not documented to be possible, or change the documentation to allow null, despite the data model not allowing null.
In other words, my @todo was something to figure out in this issue, not a future issue.
We didn't make another issue AFAIK. The current state of the patch is "for review" to figure out next steps.
I could continue dealing with any security issues on the drupal 7 branch, but that's all I've had time for since taking on co-maintainer role, not other bugs or feature requests, or other branches.
Postponing on ✨ Provide the ability to instrument the queue system Active
I can't tell from your description exactly what happened. Maybe there was an error that lead to circular reference, and once you fixed it the circular reference disappeared? But it sounds like maybe there was no actual error, you were trying to do something totally valid which lead to the circular reference?
Either situation is not good...
Because of how the raven logger (unfortunately) has service dependencies, it's difficult to completely avoid the potential for circular references. But when they are found we do try to find a way to eliminate them.
It would be great to resolve this issue, because Bluesky RSS feeds do not have a title.
I'm reclassifying this as a bug, because it seems like a good idea to support Bluesky's RSS feeds.
Please provide steps to reproduce from a clean install of Drupal so we can look into this. For example, install module X, enable Y on the settings page, add Z to the settings.php file, etc.
Ok, glad that works for you.
I do think there is some potential for improvement here, like checking $context['@message'] for matching ignored message, if it exists, to make it easier for non-developers to ignore messages..
That said this module is really targeted at developers who can do a lot of customization in code...
Sorry for the miscommunication: Subscribing to the event and setting the before_send callback is something you should do in your custom code to provide the functionality you're looking for.
It should not be done in this module.
Thinking thru some pros and cons of various solutions for this:
If we check both the unformatted and formatted message in ignored_messages, then it would be easy to ignore specific errors. However, whenever the line number or absolute path of the file changes, the error would no longer be ignored. Therefore, this would be a pretty brittle solution - it doesn't seem particularly great to me
Another idea would be to check both the unformatted message and, if it exists, $context['@message']. This would be a more general rule, ignoring new cases of the error that appear, regardless of where it appears. So, it seems more useful as a filter, although certainly could cause people to unintentionally miss new errors.
We intentionally match the generic unformatted/uninterpolated message because that is often necessary. For example, we don't want to have to ignore a message for each user logging in, but rather for "[user] logged in". So, we can't make this change.
To filter events the way you want you'll need to write a little code to set the before_send callback option, which allows you to provide arbitrary custom logic for filtering events; the ignore_exceptions option is also available.
It looks like Drush has its own queue runner code, so would need to duplicate the event dispatching code?
These would be my proposed events to enable instrumenting:
When an item is added to a queue: ItemQueuing, ItemQueued
When a queue item is being processed: ItemProcessing, ItemExceptionOccurred, ItemProcessed
Ready for review - this removes some potential edge case behavior changes where null would have been cast to 0.
But, the methods are not documented to return null. So, I think either a deprecation should be triggered if null is returned, or the documentation could be changed to allow null, despite the data model not allowing null.
Updated the change record
As for the instrumentation, this shouldn't have any influence on application code. I have been evaluating contrib-auto-pdo and this is recording spans for database queries / statements from the very beginning of a request / response cycle if configured properly. No need for events or other application level code in that case.
Very nice! I might play around with this myself. Note the only reason this works is because of the opentelemetry PHP extension.
I'd like to see it built in to core - presumably by dispatching events - so no overriding would be necessary. So next step could be to file a core feature request
Did you have a concept for how to instrument the queue API? I didn't see anything obvious last I looked, unlike Laravel which fires events as jobs are processed. Might be necessary to alter/override the queue service and/or work on adding instrumentability to core queue?
This module already includes HTTP request instrumentation. Are you finding that it's not working?
Drupal 11 is already supported
Altering JS configuration is already documented to some extent in the README file, but feel free to make improvements
Then sounds like this can be closed? Feel free to re-open if you have more info
@joseph.olstad I wasn't able to reproduce this issue; please write a failing test or update the steps to reproduce
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.