Move feature request to current dev branch.
I believe you would now need to call something like Sentry.addIntegration(integration: Sentry.replayIntegration());, as we now use a callback function for drupalSettings.raven.options.integrations rather than a simple array.
Move feature request to current dev branch
Move feature request to current dev branch
Move feature request to current dev branch
In a followup issue we may need to add a log filtering feature? Although, developers can do their own filtering via Sentry callbacks.
For example, maybe by default, logs could honor the same "ignored channels" and "ignored messages" that errors do, with a checkbox to capture all logs, including ignored channels and ignored messages? There might even be a use case for logs having its own lists of ignored channels and ignored messages.
Actually fixed on the 7.x branch
Actually fixed on the 7.x branch
Actually fixed on the 7.x branch
mfb → changed the visibility of the branch 3541436-backport-3 to active.
mfb → changed the visibility of the branch 3541436-backport-3 to hidden.
This likely needs a backport
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?