@urix I'm still unable to reproduce this issue. I need steps to reproduce from a clean install of Drupal 10 or 11.
Perhaps there is a contrib module or particular configuration that somehow prevents the field from being uninstalled, and steps to reproduce would give me a hint what that is and allow me to debug it.
Let's keep this open for documentation improvements
@jdhildeb is there any particular reason you need filehash 3.0.x as a dependency?
There should be no difference between 3.0.x and 3.2.x, compatibility wise. Each minor version just adds a new feature, such as a new Drush command or Views filter.
I would like to write test case for this so it would be useful to know how it got into that state. If already installed then an update would have updated the config schema?
In any case, safely handling null config is a good idea in case someone manually removes a config, for some reason.
Do you get the same error on one of the current versions of filehash?
I typically put logic in the before_send callback itself so it is checked at the last minute, i.e. current maintenance mode, current route, current user, etc.
And, check how logging from drush commands or other CLI scripts is disrupted.
One thing to keep in mind is that drush commands might also run as the anonymous user.
You should not need a feature to provide this logic. Add an event listener for the \Drupal\raven\Event\OptionsAlter event, and set the before_send callback option to a function containing whatever logic you need:
function (\Sentry\Event $event, ?\Sentry\EventHint $hint): ?\Sentry\Event {
}
Some more type questions:
It looks to me like value and description are permitted to be render arrays, not just Stringable? (when testing this out in a legacy requirements hook)
I think value could also reasonably be a numeric type? In case someone wants to print a number without units.
I think that's not the correct interface to use? I get t('') instanceof \Drupal\Core\Entity\TranslatableInterface === FALSE
Maybe just string|\Stringable is fine, not my area of expertise though.
I guess you're implying that all the keys are optional? Which isn't what the existing docs say, but I've not tested it out.
Added a functional test - but for now, it's going to be failing because I added a test that the logger can be serialized.
Looks like
moduleHandler()->invokeAll()
can make database queries - cache gets - which I can't imagine you'd want to do as part of dispatching events inside the database layer.
I'm not sure how to dynamically register a hook implementation so that we can make a nice little test script, but we can add an event listener and dispatch an event, and then in comparison, invoke a hook that doesn't exist:
$bytes = memory_get_usage();
$nanoseconds = hrtime(TRUE);
Drupal::service('event_dispatcher')->addListener('my event', fn () => print 'EVENT: ' . PHP_EOL);
Drupal::service('event_dispatcher')->dispatch(new stdClass(), 'my event');
echo (hrtime(TRUE) - $nanoseconds) / 1000, ' microseconds', PHP_EOL;
echo memory_get_usage() - $bytes, ' bytes', PHP_EOL;
echo 'HOOK: ', PHP_EOL;
$bytes = memory_get_usage();
$nanoseconds = hrtime(TRUE);
Drupal::moduleHandler()->invokeAll('not actually a hook');
echo (hrtime(TRUE) - $nanoseconds) / 1000, ' microseconds', PHP_EOL;
echo memory_get_usage() - $bytes, ' bytes', PHP_EOL;
Do you have a source for that? I don't see how that is possible.
In my testing, Drupal::moduleHandler()->invokeAll() is slower than Drupal::service('event_dispatcher')->dispatch()
Both the first call, and the second call are slower.
Events are of course heavily used in low-level code, for example, request events/event subscribers. So that seems like the obvious building block we'd use to allow for instrumenting various low-level subsystems. And we already are using as mondrake mentioned.
And I wouldn't mind having some new events for instrumenting how slow the hook system is 🙃
Dispatching an event is faster than invoking a hook (and uses less memory), so it doesn't make sense for low level code like the database layer to invoke extra hooks, especially in the context of performance analysis.
I think there is some chance of loggers being serialized in various contrib and custom code (if not in core). See e.g. the dblog logger, which uses DependencySerializationTrait. So, service closures would only be usable here if/when they can be serialized.
Added the service closure. Another possibility, I guess, is to actually use the config property via __get() as a shorthand way to call ($this->configFactory)()->get('syslog.settings') :) But I didn't do that yet, just added a deprecation
Ok, since this issue is progressing slowly, I added a proposed resolution D) to the issue summary, to fix the SysLog constructor. It's definitely correct to make that fix in the short term, but AFAIK, it won't help with edge cases where a ServiceCircularReferenceException is thrown at the time the logger service is being instantiated (due to the config dependency). Lazy services would help with many of those edge cases. Or establishing policy that loggers should not depend on configuration would be the nuclear option, with some usability impacts for site admins (and BC issues).
I read thru this issue but I don't understand it. A circular reference re: syslog module when syslog module is not installed? Can someone provide steps to reproduce?
This needs an issue summary update, as I'd say it should be about hash algorithm extendability, with the example use case provided by a separate contrib module.
I couldn't figure this one out. Please re-open the issue if you can provide more info on how to reproduce the problem. Thanks!
In practice, I would say that using a 255-character truncated description as the title is probably too long for most sites. I'd suggest making it configurable, or at least decreasing it somewhat
would be nice to roll a release some day soon, the last release was August 2024
Only Olivero as far as I know - it's a bug in Olivero's tabs.js
I wasn't able to reproduce this issue - please re-open if you can provide more info to help me reproduce (and fix) the problem.
This module doesn't care what endpoint you use, it could be your own install of Sentry or any of the other apps that are compatible with Sentry SDK. That info is for you to update your firewall rules.
Interesting, so is that the correct place for new theme tests?
I agree test coverage would be excellent. Note that apparently there is no javascript test coverage at all for Olivero theme (and only two test methods overall).
@trevorbradley that doesn't seem related to this issue, as far as I can tell. This issue has to do with the display of log entries by dblog module. Your issue has to do with LogMessageParser. Because of the fact that log messages/placeholders are "parsed" by LogMessageParser, json should be in the placeholders rather than part of the log message. Aside from being technically required, this makes more sense anyways, since json will typically be something that changes often, whereas messages are more like a generic, translatable template.
You should only be seeing an event related to "send_monitoring_sensor_status_changes" if the sensor status changed.
If you're seeing it often, that would imply that the sensor status is changing often - i.e. either you've misconfigured the sensor with regards to how you are running cron, or something is actually wrong with your cron runs, right?
For example, let's say you have configured the "Last cron run age" monitoring sensor to warn you if cron hasn't run for two hours. If cron was previously working ok, but now it hasn't run for two hours, then the sensor status changes to warning, and you get a Sentry event (with severity = warning). If cron runs shortly before the next sensor run, then the sensor status will change to OK and you get another Sentry event (with severity = debug). If the two-hour configuration makes sense for what you're doing, and everything is working normally, then you shouldn't be seeing such sensor status changes.
Unless you've hit a bug of course - but I've not seen one myself. Setting this to postponed, needs more info since I would need more information to reproduce a problem.
An extendable, plugin re-architecture sounds reasonable; if folks are interested in working on that I could review it. At the moment we have a non-extendable enum architecture.
Presumably these particular features should live in a separate contrib project, since they would have additional dependencies?
Meaning this issue should be re-scoped to be an extendability feature request.
Thanks for the review
re: making it configurable in the UI - this would need a bit more work / contribution from someone. Possibly it could even be two configurations, one for strings and one for regular expressions?
This doesn't add support, but does document the existing support
js_settings_alter is for altering "normal" settings, which are json data. But a callback function is js code, not json data so it would be handled differently. I did mention this in the README but feel free to contribute documentation improvements.
Ok great, please update the issue if you still have problems.
If you do want to set more advanced options such as beforeSend, ignoreErrors etc. you can add those more easily via a custom javascript library (I'm unclear how the javascript code you added to settings via PHP would get evaluated).
Interesting. It does seem to work for me. What type of JS error reports are you still receiving?
I have not tried it but I would assume that disabling defaultIntegrations should work, as tracing is not one of the default integrations.
(Which could be done either in JS, or on the PHP side by attaching the setting to the page or via the js_settings_alter hook..)
Not something I've tried, but it might be enough for you to set drupalSettings.raven.options.defaultIntegrations = false;?
I just double-checked this and there is no change between 11.2 and the 11.x branch except that the TypeError is thrown. BigPipe JS also can't find a placeholder in a <noscript> tag in 11.2 - I had never actually noticed this fact before, since it wasn't a problem.. :)
I would assume that if something has already been replaced server side, then it would not be sent to the browser, so would not be a target at all.
Yes that would be correct. The exception is only thrown in the browser when it was not done server side.
There is no way to tell big pipe "only do this replacement server side because it's in a noscript tag", but such complexity isn't needed. We can just keep things working like they did historically: don't throw if the browser can't find a placeholder.
Actually I realized my attempted work-around I mentioned in #6 doesn't work, because the noscript tag is added to the dom, and whatever is inside actually loads. So, I think my MR is the best solution: bigpipe javascript should just ignore any situation where target is null.
Markup in a noscript tag is by definition already special-cased, because it's not interpreted as being part of the dom. And since it's only relevant when javascript is disabled, handling replacement on server side is fine.
I don't think there anything special that bigpipe should do, except just not error in any rare cases where the target can't be found.
I think this would mean that no other placeholder strategy would find the placeholder either.
Not sure what you mean by the - the non-javascript placeholdering has always worked fine - the placeholders are replaced on server side.
I did find a work-around I could use on Drupal 11.3 and later: I can instead have the noscript tag be part of the replacement markup, instead of wrapping the placeholder. Then, of course the javascript can find the placeholder, because it's not in a noscript tag.
However, rather than having to work around the issue, I think it would be best to avoid throwing here - just don't try to process the target if it can't be found.
It does not seem a matter of checking the value of file_size in the template file, but rather to know which formatter is using that template file, or use two different template files.
See the following paragraph of the issue summary - this should be handled by 🐛 'file_table' formatter shows file size twice Active (a new size toggle available to formatters or other callers).
I don't see a need to update the template docs because the with_size variable isn't used there. (It /could/ have been used there, if we moved the logic from the preprocess function into the template, but I didn't put the logic there to minimize the need for updating templates.) I assume templates don't need to exhaustively document all possible variables.
Do we need to add a code comment in TableFormatter where we set #with_size to FALSE?
Never mind, moved the fix here in hopes of faster reviews
I incorporated a fix for this Claro bug in a MR for 🐛 'file_table' formatter shows file size twice Active , which resolves the related issue re: file module needing logic to turn off file size display when it's already displayed elsewhere.
While fixing this issue w/ file module, it seemed like a good time to also fix a related issue with claro module, which, unlike the default file link template, was the missing logic to not show the file size if it's empty.
The views filter is now released in version ^3.2, ^2.1 and ^1.12
unfortunately, since the table changed between 1.x and 2.x I don't think the same view can be used across all versions - but maybe the view could be altered on 1.x using a hook
I noticed that it recurses through .git directories too - that wouldn't be as bad as node_modules but, still seems like a waste
@carlitus this bug was fixed here in hreflang module, and was released in version 1.15
Oh cool, well, not sure how much time I might have to literally maintain this module - but the good news is, it should be quick and easy for us / someone to build the view and add it to this module.
Actually, I think it would be useful if we allow the filter to be exposed, so that the end user can turn the duplicate filter on/off? Updating issue summary to say so.
Since the parent issue was cherry-picked to 10.6.x this should as well?
@fishfree I wasn't able to reproduce this exception in my quick testing.
Does it happen for you on a fresh, clean install of Drupal core?
What version of Drupal core are you using?
Do you have any contrib modules installed?
If I'm not able to reproduce it then I won't be able to work on a fix...
I think this should also be query a optimization when there are tens of thousands of files - it can jump to the next fid rather than scanning through a large row offset.
Not sure if this was officially released but in any case I added a link
Thanks for reporting!
Already resolved by humans