Deprecate watchdog_exception

I think \Drupal makes sense.

+++ b/core/includes/bootstrap.inc
@@ -398,21 +398,13 @@ function drupal_validate_utf8($text) {
+ * @deprecated in Drupal 8.5.0 and will be removed before Drupal 9.0.0.
+ *   Use \Drupal::logException() instead.

I think we have to mention change notice link here as well.

I can't find anything to nitpick so RTBC.

+++ b/core/includes/bootstrap.inc
@@ -398,21 +398,13 @@ function drupal_validate_utf8($text) {
+  @trigger_error('watchdog_exception() is deprecated in Drupal 8.5.0 and will be removed before Drupal 9.0.0. Use \Drupal::logException() instead. See https://www.drupal.org/node/2932520', E_USER_DEPRECATED);

+++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
@@ -123,6 +123,7 @@ public static function getSkippedDeprecations() {
+      'watchdog_exception() is deprecated in Drupal 8.5.0 and will be removed before Drupal 9.0.0. Use \Drupal::logException() instead. See https://www.drupal.org/node/2932520',

Let's do this when we've removed all usages in the follow-up.

I'm not 100% sure that putting this on \Drupal is the best place for it. Really, this function does two things,

1. massage some variables and an exception suitable for use with the logger
2. Do the logging.

In OO code, I'd expect you'd inject a logger, so you really only need the first bit, but then we already have Error::decodeException for this.

I'd be interested to see how many OO calls to watchdog_exception passed the optional variables.

If not many, then those calls should be changed to just log after calling Error::decodeException and this can live on \Drupal for non OO code sake. I.e I suspect we shouldn't need this from OO code, but would like to see the stats first. If there are a few calls to it from OO code that do pass the additional variables, then there might be merit in splitting this into two separate functions as per the two things it does.

OO code would only need the first half

I'd be interested to see how many OO calls to watchdog_exception passed the optional variables.

When I looked at the code I have locally (some contrib modules + core) there was just the search API module using this functionality:

modules/contrib/search_api_autocomplete/src/Controller/AutocompleteController.php
130:      watchdog_exception('search_api_autocomplete', $e, '%type while retrieving autocomplete suggestions: !message in %function (line %line of %file).');

modules/contrib/search_api_solr/search_api_solr.module
78:        watchdog_exception('search_api_solr', $e, '%type while optimizing Solr server @server: @message in %function (line %line of %file).', array('@server' => $server->label()));

modules/contrib/search_api_solr/src/Form/SolrConfigForm.php
74:      watchdog_exception('search_api_solr', $e, '%type while retrieving config files of Solr server @server: @message in %function (line %line of %file).', array('@server' => $search_api_server->label()));

modules/contrib/search_api_solr/src/Plugin/search_api/backend/SearchApiSolrBackend.php
648:      watchdog_exception('search_api_solr', $e, "%type while indexing: @message in %function (line %line of %file).");
1989:        watchdog_exception('search_api_solr', $e, "%type during autocomplete Solr query: @message in %function (line %line of %file).", array(), WATCHDOG_WARNING);

, but even then, those usecases still fundamentally set the same error message. I think its fair to assume that all these parameters got inherited from watchdog() in Drupal 7.

For me there are a two possiblities:

1. Accept that adding an additional Error::decodeException() is not that bad actually. Code which cares about logging exceptions probably have some other logger injected already as well. All other customizations could be applied on top of that
2. Provide a new ExceptionLoggerInterface which has something like logException as its own method. Internally it could wrap the exception as wanted and maybe provide $exception, $message = NULL as possible parameters. For using it could be constructed from an existing logger, so maybe: ExceptionLogger::create($this->logger)->logException($exception)
3. We agree that a static method isn't too bad. Even in this case there is no need for adding it to \Drupal. It could happily live somewhere in \Drupal\Core\Logger for example.

Note: At least in my head 2) Wouldn't really have any additional service. watchdog_exception was not swappable, nor does ExceptionLogger::create() | ExceptionLogger::logException($logger, $exception). Does that make a little bit more sense?

While it is true that this is "more OO", I don't feel that's a solid justification for this change. It doesn't increase testability, it doesn't make code less coupled.

I don't have a solid answer to #13 either.

@kim.pepper Do you have a question about #13?

Yay! OOP! I agree with @dawehner that we can probably come up with a better place to put this than on \Drupal. That's really just a bunch of helpers for code that's procedural or has trouble with DI. The assessments that this primarily prepares values for LoggerInterface is a good one. What about we put this on a tiny helper class somewhere (I wish we did function autoloading too) as a autoloadable, non-injectable helper? The function can return a tuple-style array so calling code can call LoggerInterface itself, or the helper function also takes a severity (defaults to error?) and LoggerInterface and logs the parsed exception for you? This would save us the trouble of using another container service, unless anyone sees a need for swapping exception log message formatting application-wide.

This illustrates one of the approaches I suggested (the decorator, as PHP/Drupal folks may not be too familiar with tuples). I opted to mimic \Psr\Log\LoggerInterface, which explains the change in parameters from the previous patch, which was more closely based on watchdog_exception(), but that is more for convenience and not essentially required. This patch also includes two conversions, with one procedural (being watchdog_exception() itself) and another OOP inside ModuleInstaller.

I almost wonder if it's worth seeing if we can revise the logging PSR to accept not just strings, but anything that can be cast to a string. Then we can just decorate exceptions in objects that implement ::__toString() and pass them on to any old LoggerInterface directly.

This patch brings back the concept of a convenience method on \Drupal that @kim.pepper originally introduced to provide a much cleaner procedural alternative to watchdog_exception() than what #19 allowed us to do. If we want, we can keep track of exception loggers per log channel inside \Drupal to limit the number objects.

Updating the issue summary. I do like the patch right now.

+++ b/core/lib/Drupal.php
@@ -708,6 +709,20 @@ public static function logger($channel) {
+  /**
+   * Gets an exception logger.
+   *
+   * @param string $channel
+   *   The name of the channel. Can be any string, but the general practice is
+   *   to use the name of the subsystem calling this.
+   *
+   * @return \Drupal\Core\Logger\ExceptionLogger
+   *   The exception logger.
+   */
+  public static function exceptionLogger($channel) {
+    return new ExceptionLogger(static::logger($channel));
+  }
+

May I ask whether it would be possible to use ExceptionLogger::create() instead and skip the method on \Drupal altogether.

Thanks for the review! :) The reason this is on \Drupal is that there's where most of the 'procedural-to-OOP' shortcuts live. It would also allow us to statically cache exception loggers if we want to (maybe not....yet?). It also makes it as similar to regular loggers as possible, although I'm fine with making this whole thing more minimal and look less like LoggerInterface. What would be the benefit of ExceptionLoger::create() over ::__construct()?

) The reason this is on \Drupal is that there's where most of the 'procedural-to-OOP' shortcuts live.

Yeah I guess it seems totally fine.

It would also allow us to statically cache exception loggers if we want to (maybe not....yet?).

I think optimizing for the exception path seems like a bad optimization strategy :)

What would be the benefit of ExceptionLoger::create() over ::__construct()?

I think the first is more readable, but also enables future changes of the constructor, your caching if needed.

Small cleanup to issue summary

From #2351099: watchdog_exception() hides the exception from logger implementations, the exception should be passed as a value in the context array of the log call.

+++ b/core/lib/Drupal/Core/Cron.php
@@ -236,7 +236,7 @@ protected function invokeCronHandlers() {
-        watchdog_exception('cron', $e);
+        \Drupal::exceptionLogger('cron')->error($e);
       }

May I ask a question about the API? Is there ever a usecase for doing ExceptionLogger::info() for example? If ExceptionLogger doesn't actually implement the logger interface anyway, wouldn't it be enough to have ExceptionLogger::logException and not have any of the additional methods?

I don't think it makes sense to introduce a new service only for logging exceptions, when the existing logging service can be improved (as in #2949419: Pass the raw exception to logger implementations), and doing so would conform to the expectation of other PSR loggers.

Unless I'm missing something, that's just a minor feature addition to the existing procedural code.

This issue is about replacing the procedural function with a service.

How is that issue relevant to this one?

Then shouldn't this issue be repurposed to still deprecate watchdog_exception() and then pass the exception once #2949419: Pass the raw exception to logger implementations lands?

In any case, either in that issue or this one, watchdog_exception() needs to be deprecated and its implementations removed, yes?

Well, that issue is kind of separate from this one. This one specifically deals with the deprecation of watchdog_exception(). I don't think we should muddy that issue with this now that I think about it.

I'd say the current service approach just needs to be removed from the latest patch. The deprecation and replacement of the watchdog_exception() usages is still valid.

Although, this all being said, I don't think we can do this in 8.9.x... so moving to 8.8.x if it can get in. Otherwise, I suppose it'll have to be bumped to 9.0.x?

Currently, DbLog stores everything passed to $context (except the removed backtrace) to the variables column of the log table.
The patch for #2949419: Pass the raw exception to logger implementations updates DbLog to also remove the exception object from being stored in the database.

#2949419: Pass the raw exception to logger implementations also updates Error::decodeException() to include the passed exception in the variables array it returns, so the only thing really missing that watchdog_exception provides is a default message.

I used a regex to find/replace instances in the last patch, so missed a couple that didn't match the same format.

Some missing imports for the Error utility class where it's now used, and make use of a service's injected logger instance where available instead of using the static Drupal::logger()

1. Migrated a couple new instances of watchdog_exception() over to \Drupal::logger().
2. Fixed a minor typo in the issue summary.

Also, should we really be preemptively unseting the 'exception' offset, which does not exist, in update.inc for the sake of an issue which has yet to be accepted into core? (i.e. #2949419: Pass the raw exception to logger implementations)

+++ b/core/includes/update.inc
@@ -188,11 +188,11 @@ function update_do_one($module, $number, $dependency_map, &$context) {
       $variables = Error::decodeException($e);
-      unset($variables['backtrace']);
+      unset($variables['backtrace'], $variables['exception']);
+      $ret['#abort'] = ['success' => FALSE, 'query' => t(Error::DEFAULT_ERROR_MESSAGE, $variables)];
 
@@ -255,13 +255,13 @@ function update_invoke_post_update($function, &$context) {
       $variables = Error::decodeException($e);
-      unset($variables['backtrace']);
+      unset($variables['backtrace'], $variables['exception']);
       $ret['#abort'] = [
         'success' => FALSE,
-        'query' => t('%type: @message in %function (line %line of %file).', $variables),
+        'query' => t(Error::DEFAULT_ERROR_MESSAGE, $variables),
       ];

The replacement recommendation should be updated to use Error::DEFAULT_ERROR_MESSAGE and Error::decodeException, as is done in the rest of the patch.

#2949419: Pass the raw exception to logger implementations already includes the changes with unset to remove the exception variable, so those could be removed from this patch.

Okay, the replacement I recommended in the previous patch already makes use of Error::DEFAULT_ERROR_MESSAGE and Error::decodeException().

I removed unseting of the aforementioned exception offset, since the change is already included in #2949419: Pass the raw exception to logger implementations.

+++ b/core/includes/bootstrap.inc
@@ -430,12 +430,18 @@ function drupal_validate_utf8($text) {
+ * @deprecated in drupal:8.8.0 and will be removed from drupal:9.0.0. Use
...
+  @trigger_error('watchdog_exception() is deprecated in drupal:8.8.0. It will be removed from drupal:9.0.0. Use \Drupal::logger() and pass the exception as a variable instead. See https://www.drupal.org/node/2932520', E_USER_DEPRECATED);

I assume this should be updated to 8.9 now? Probably 9 -> 10 too? I don't remember off the top of my head but I would wager we can't remove this in 9 at this point.

actually, no there's a 8.9 beta out so without special consideration this is 9.x and 10.x.

Updated patch and change record to target 9.1.x and 10.0.x due to there being both a 8.9 and 9.0 beta out as of this comment.

It is a bit frustrating that this will have to be postponed until 9.1.x due to the lifetime of this ticket and it not being disruptive to my knowledge 😞. Though I do understand the necessity of not introducing new patches late in the release cycle.

I'm with you. I mostly keep coming back here like "oh, you still can't do that? this never got committed? How could that be?"

The first bot failure was github being down but all the requeues I've tried failed to apply. I tried every possible method I know of applying patches and they all worked so I feel like this is a testbot failure but I'm feeling a bit out of the loop and/or noobish because everything I've been posting the last couple days seems to have weird mysterious failures I can't figure out.

That was my bad, I was still targeting 8.9.x for the test even though I had updated the ticket and patch to target 9.1.x 😅. I just added a test to target 9.1.x.

Transitioning back into "Needs review" since it passed the 9.1.x test.

Re-roll for 9.1.x.

Thanks for working on cleaning up our legacy code! A few points of review:

1. +++ b/core/includes/errors.inc
   @@ -185,7 +185,7 @@ function _drupal_log_error($error, $fatal = FALSE) {
   -      $response->setContent(html_entity_decode(strip_tags(new FormattableMarkup('%type: @message in %function (line %line of %file).', $error))) . "\n");
   +      $response->setContent(html_entity_decode(strip_tags(new FormattableMarkup(Error::DEFAULT_ERROR_MESSAGE, $error))) . "\n");
   
   @@ -196,7 +196,7 @@ function _drupal_log_error($error, $fatal = FALSE) {
   -        $response->setContent(new FormattableMarkup('%type: @message in %function (line %line of %file).', $error));
   +        $response->setContent(new FormattableMarkup(Error::DEFAULT_ERROR_MESSAGE, $error));
   
   @@ -236,7 +236,7 @@ function _drupal_log_error($error, $fatal = FALSE) {
   -        $message = new FormattableMarkup('%type: @message in %function (line %line of %file).', $error);
   +        $message = new FormattableMarkup(Error::DEFAULT_ERROR_MESSAGE, $error);
   
   +++ b/core/lib/Drupal/Core/Utility/Error.php
   @@ -18,6 +18,11 @@ class Error {
   +  /**
   +   * The the default message for logging errors.
   +   */
   +  const DEFAULT_ERROR_MESSAGE = '%type: @message in %function (line %line of %file).';
   +
   

   Can we call this DEFAULT_ERROR_MESSAGE_TEMPLATE or something? I got really confused when we were passing a constant to FormattableMarkup.

   I'm also wondering if a defaultError(array $error_variables) method would be a better way to handle a default message format, rather than a constant?

2. +++ b/core/includes/update.inc
   @@ -220,11 +220,11 @@ function update_do_one($module, $number, $dependency_map, &$context) {
   -      $ret['#abort'] = ['success' => FALSE, 'query' => t('%type: @message in %function (line %line of %file).', $variables)];
   +      $ret['#abort'] = ['success' => FALSE, 'query' => t(Error::DEFAULT_ERROR_MESSAGE, $variables)];
   
   @@ -287,13 +287,13 @@ function update_invoke_post_update($function, &$context) {
   -        'query' => t('%type: @message in %function (line %line of %file).', $variables),
   +        'query' => t(Error::DEFAULT_ERROR_MESSAGE, $variables),
   

   t() can't translate a constant, which is another reason against using a constant. We could have translatedDefaultError() and untranslatedDefaultError(), or something. There might be better names.

3. +++ b/core/lib/Drupal/Core/Database/database.api.php
   @@ -188,7 +188,7 @@
   - *     watchdog_exception('type', $e);
   + *     \Drupal::logger('type')->error('%type: @message in %function (line %line of %file).', Error::decodeException($e));
   

   This seems to not have the constant (that I think should be a method).

I don't see the point of using t() in error messages. Pretty sure we aren't doing this anywhere else in core. Why here?

I don't think it is being used for "errors." Like we normally would. It looks like this is trying to pass something to the update UI. I can't find any direct usage of ['#abort'] other than to check its existence though so maybe previous reviews couldn't figure it out and left it alone instead of converting to FormatMarkup? Or maybe the message is completely dead? update.inc is really hard to follow :( :(

dawehner wrote this for 8.0 in #2538108: Add hook_post_update_NAME() for data value updates to reliably run after data format updates. Maybe he remembers something...

@neclimdul You're right, it's not being used as a watchdog error message; though the message is still in use. See lines 457-460 of DbUpdateController.php for reference.

$messages[] = [
  '#wrapper_attributes' => ['class' => ['failure']],
  '#markup' => '<strong>' . $this->t('Failed:') . '</strong> ' . $query['query'],
];

$query contains the value of $ret['#abort']. When an update fails, a translated version of the message being logged is displayed to the user stating where the update failed at.

I agree with @kim.pepper that this seems a bit unnecessary since error messages being logged to watchdog currently aren't being translated.

Hope this helps!

@abhijeet.kumar2107 Thank you for expressing interest in this ticket! I'm unassigning this ticket in accordance with this entry from the Drupal.org community wiki:

Contributors to Drupal Core are discouraged from using the "assigned to" field unless they have a very good reason to use it. In most cases it is more appropriate to create a comment indicating that you are working on the issue and specifying which part of the issue you are working on. Then update the issue from time to time with reports on any progress made or roadblocks encountered.

@xjm @kim.pepper in response to #75 and #76, could we potentially leverage the already existing Error::renderExceptionSafe() method which is being used to accomplish a very similar task and just make inclusion of a backtrace optional using a second argument as a flag? Something like:

  public static function renderExceptionSafe(\Throwable $exception, bool $include_backtrace = TRUE) {
    $decode = static::decodeException($exception);
    $error_message_format = self::DEFAULT_ERROR_MESSAGE;
    if ($include_backtrace) {
      $error_message_format .= ' <pre class="backtrace">@backtrace</pre>';
      $backtrace = $decode['backtrace'];
      // Remove 'main()'.
      array_shift($backtrace);
      $decode['@backtrace'] = Error::formatBacktrace($backtrace);
    }
    // Remove unnecessary replacement variables.
    unset($decode['severity_level'], $decode['backtrace'], $decode['@backtrace_string']);
    return new FormattableMarkup($error_message_format, $decode);
  }

By making the second argument optional and default to TRUE, we can maintain backwards compatibility with existing calls to Error::renderExceptionSafe() and consolidate the implementations, rather than creating another method to accomplish the same task only without a backtrace.

Okay, just a question for clarity then: If we create a translatedDefaultError() method should we call t() a second time at the call site or pass the substitutions directly to the new static method.

Using the first example from update.inc:

// Original
$ret['#abort'] = ['success' => FALSE, 'query' => t(Error::DEFAULT_ERROR_MESSAGE, $variables)];
// Without passing substitutions
$ret['#abort'] = ['success' => FALSE, 'query' => t(Error::translatedDefaultError(), $variables)];
// Passing substitutions
$ret['#abort'] = ['success' => FALSE, 'query' => Error::translatedDefaultError($variables)];

I believe the suggestion is to just use the return from the method without another call. The problem wouldn't be that t the function can't translate the input, At runtime the constant will go into it as a string like any other which is why tests pass. The problem would have to be in how we generate translations and that static analysis I guess doesn't handle constants.

I think that's kind of a weird arbitrary limitation but that might be beside the point. Something feels kinda off about that sort of method specifically for the installer. At the very least translating errors is something we never do with this one weird exception. Why does this exception exist? If I remember correctly we don't translate errors so you can google them consistently regardless of your language. I don't know why the installer would be an exception to this rule and maybe it shouldn't be.

#73 no longer applies to D9.1.x. Here is a re-roll.

Doesn't look like that re-roll worked. Here's another attempt. Most of the conflicts where from #2807743: Switch from deprecation notice to warning for non-standard placeholders in FormattableMarkup::placeholderFormat() and #2949419: Pass the raw exception to logger implementations which where also testing error/watchdog stuff. Additionally I think a small use conflict somewhere.

Also bumped the version in the deprecation to 9.2.x since that's where this is at.

Looking back, xjm's feedback in #75 hasn't been addressed.

I did some quick digging into the "do we translate errors elsewhere" open question. Seems yes but its complicated.

https://localize.drupal.org/translate/languages/hu/translate?project=dru...

From what I can tell, anything going through the logger also gets translated? Does this also not support constants/variables? That would be pretty frustrating and easy to mess up. How do we answer that? I don't know how translatable strings are parsed out of core anymore.

Just to clarify on that last question, the issue is exactly what you were saying in #85; the guideline to only enter the "raw" English text in translation function is because of the way these originals are extracted from source. I am not intimate with the code responsible (e.g. in the PotX project) but I think "static analysis" is giving it more credit than it deserves :) I think it is basically just doing a regex match on a handful of patterns. One of those is log entries, because DB log (?) will translate error messages. I'm not actually positive it is DB log where that happens, but it would seem a logical place and I do know for a fact my Dutch sites all have translated log entries in the recent logs :)

Rerolled #89 against 9.3.x
Updated deprecation message
Added more replacements

Looks like new loggers (services) could use separate issue

@andypost which services? Been a while since I've looked at this but I don't see any new services, this is just replacing existing calls really.

Sorry, probably I did read wrong patch

btw How to restart test? the failure looks unrelated

Please, review the MR. The test based on the MR was passed.

To "needs work" for the unresolved thread on the MR.

Addressed #101

All code changes look good to me.
The only thing I am now missing is a deprecation message test for the deprecated function. Sorry, I did not check for it earlier.
After that it is RTBC for me.

All code changes look good to me.
The Is and the CR are in order.
For me it is RTBC.

The

9.4.0 or 10.0.0 and deprecated in Drupal 11

is not correct. I've chatted with the release managers and confirmed that we can deprecate in Drupal 9.4 and remove in Drupal 10.

Let's do that here - it means we have one whole release less carrying around deprecated code. We should only deprecate for Drupal 11 now if something is super-disruptive. This change is not. You can remove the usages already in 9.3.x in contrib and custom code because everything you need already exists.

Can we get one patch deprecating in 9.4.x and one patch removing everything for 10.0.x. Thanks!

Hm. It might not be complicated to replace, but it's a function that is used a lot. Not just in contrib (38 pages of results on http://grep.xnddx.ru/search?text=watchdog_exception&filename=), it's also going to be used a lot in custom code as it's the default thing to do in a try/catch.

And all it costs us to keep is a 15loc function in a file that we won't be able to remove in D10 anyway.

And last, it's going to cause more logger channels to be injected in a way that's bad for performance (factory services or calling getLogger(), which is bad for performance. FWIW, the current patch doesn't actually use DI for logger/logger factory in any place that still used this.

So IMHO, we should not push this.

You don't have to, you can just inject the logger factory and get the channel only when needed. But it's a pretty common pattern to do that already in the constructor as it looks cleaner. Or define a service with a factory which is pretty common in core and copied a lot. Most of those logs are triggered very rarely, so anything we do already upfront is a tiny to a little bit more expensive, yes.

I'm not saying that means we shouldn't do it, just that there is IMHO not a good reason to force this for a D10 removal, there's very little benefit (and even a tiny bit of extra cost depending on how it's done) to force everyone to remove it in the next few months.

Also, it's actually not really backwards compatible to do now, not if you want to use that new constant?

Yes it'd be nice to wait until the new constant is available to ask module developers to update their exception logging code (the replacement code is fairly verbose even w/ the new constant :)

You don't have to, you can just inject the logger factory and get the channel only when needed. But it's a pretty common pattern to do that already in the constructor as it looks cleaner. Or define a service with a factory which is pretty common in core and copied a lot. Most of those logs are triggered very rarely, so anything we do already upfront is a tiny to a little bit more expensive, yes.

I think we need make it easier to do the correct thing with injecting loggers. It should be possible to inject '@logger.channel.php' and it not be unnecessarily costly if it is not used.

@kim.pepper - I'm not sure - I think we might need to re-evaluate the logging system architecture. It feels as if the architecture is wrong if injecting a logger is viewed as something that is expensive.

In my testing, there is some minor memory and time savings if you can complete a request without any loggers/logger channels being initialized. But I'd assume there is lower hanging fruit as far as performance fixes go. :)

With database logging as the only enabled logger, injecting the first logger channel takes 8376 bytes memory and ~178 microseconds. These numbers increase with each logger module that is enabled.

Injecting an additional logger channel was another 616 bytes of memory and ~9 microseconds, so these are very low impact.

As far as when to commit this, I was thinking it would be excellent if the new constant could go in ASAP - like, a 9.3.x point release, if that's allowed? That way developers have the option to start fixing their error logging code now, even before the function is deprecated.

#3260044: Add a constant for watchdog_exception message has landed.
The MR needs a reroll.

Removing procedural code is good, but watchdog_exception() was nice DX. We're replacing a single call with a fair amount of boilerplate.

Couldn't we have something like:

\Drupal::logger('update')->logException($exception);

Re #121, #122 @joachim and @kim.pepper: Just had EXACTLY the same idea! That would be really a great DX solution! +++1 :)

The proposed solution from https://www.drupal.org/node/2932520 is a drawback.
My hope was that
\Drupal::logger('update')->logException($exception); was already existing...

> Were are using the PSR-3 Logger interface, so we can just add new methods to it.

I can't tell whether that is a typo or not -- should it say "we CAN'T add new methods" or that we can?

If we can't, then we can still make the DX better, by using the splat operator to unpack the arguments to error(), like this:

\Drupal::logger('update')->error(...Error::decodeException($exception));

// decodeException changes to return an array of all the params required by error()

That makes it a 1-liner and no need for the constant for the message string.

Seems like changing the decodeException return array that way would be an unpleasant BC break (although it is of course allowed in a major version); a new method seems less prone to cause problems (code that is compatible across versions can check if method exists just as it would check if the constant exists).

I couldn't remember if decodeException() was new in this issue or already existed. Another method, in the latter case. The use of the splat with a helper method which provides all the parameters to error() is the main thing :)

I did a reroll on the MR to make it mergeable again, then created a new method (#127) to pass the error using the splat operator (as suggested in #124 and #126)

I've rebased this but not looked at the test failure.

We're against 10.1.x at this point; new MR squashing the prior changes and updating the dangling unreconciled diff.

I'm not quite familiar enough with the testing environment to know how to fix the phpstan error:

Instantiated class Psr\Log\Test\TestLogger not found.

> We're against 10.1.x at this point; new MR squashing the prior changes and updating the dangling unreconciled diff.

Please don't do squashes of commits. It loses history, and it makes it harder to do future rebases.

I'll clean up the rebase conflict as soon as I have time.

I've fixed the stray merge conflict and pushed to branch 2932518-deprecate-watchdogexception. That's still on 10.0.x, for the benefit of anyone wanting to patch their site.

I've then made a new branch rebased onto 10.1.x. The correct way to do this without using squash is:

> git rebase --onto 10.1.x 10.0.x

The new branch is 2932518-deprecate-watchdogexception-10.1. I've made a new MR for that.

Could the MR with the squashed code be closed?

Setting to needs work based on review comments.

Please make new commits on 2932518-deprecate-watchdogexception-10.1 (and optionally cherry-pick to 2932518-deprecate-watchdogexception).

ARGH I am *sure* I'd cleaned up those merge conflict markers that got left in!!!

Just cleaned up and pushed.

I looked at the test failures. One was the conflict markers.

The other is this, which I don't understand:

------ ----------------------------------------------------------------------
Line core/tests/Drupal/KernelTests/Core/Bootstrap/LegacyBootstrapTest.php
------ ----------------------------------------------------------------------
27 Instantiated class Psr\Log\Test\TestLogger not found.

I can see the class Psr\Log\Test\TestLogger in the codebase!

I rerolled the last patch.

Doing a "re-roll" on to a new MR is not helpful. Addressing the issues raised in #140 would be a positive contribution.

 ------ ---------------------------------------------------------------------- 
  Line   core/tests/Drupal/KernelTests/Core/Bootstrap/LegacyBootstrapTest.php  
 ------ ---------------------------------------------------------------------- 
  27     Instantiated class Psr\Log\Test\TestLogger not found.                 
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols   
 ------ ---------------------------------------------------------------------- 

@joachim I asked @xjm and told me that class was removed from the psr/log version that Drupal is using (PSR-4), so that's why the error.

Could we use this ?
https://nerdpress.org/2022/03/04/class-psrlogtesttestlogger-not-found/

See #3321905: Add colinodell/psr-test-logger to core's dev dependencies. Do we need to postpone on that issue?

Current MR diff for 9.5.x, if you're like me and not yet on D10 and can't use the MR diff for your project yet.

Sigh... that's not right at all.

This is for 9.5.x users.

We don't deprecate new things in 9.5.x anymore, so hiding the patch files. Also closed the redundant MRs for clarity. Thanks!

We don't need to add a test dependency, I don't think; we can construct our own test logger. Edit: although the related issue for auto updates seems to express a different opinion.

Updated for latest 9.5.x, not for review but reference.

#122 you tagged for issue summary update that still needed?

Patch #157 appears to have some failures.

The service "drupal.proxy_original_service.module_installer" has a dependen
20:35:49 cy on a non-existent service "logger.channel.system". Did you mean one of t
20:35:49 hese: "logger.channel.default", "logger.channel.php", "logger.channel.image
20:35:49 ", "logger.channel.cron", "logger.channel.file", "logger.channel.form", "lo
20:35:49 gger.channel.security"?

Assuming this no longer needs a reroll.

Only think I saw was that there are 2 instances of

is deprecated in drupal:10.0.0

But the others are deprecation in 10.1

Shouldn't they be the same?

Patch cleaning up DI and fix #162

1. +++ b/core/lib/Drupal/Core/EventSubscriber/MenuRouterRebuildSubscriber.php
   @@ -87,7 +88,7 @@ protected function menuLinksRebuild() {
   -        watchdog_exception('menu', $e);
   +        \Drupal::logger('menu')->error(...Error::decodeExceptionWithMessage($e));
   

   I think logger needs injection here

2. +++ b/core/lib/Drupal/Core/Routing/MatcherDumper.php
   @@ -152,7 +153,7 @@ public function dump(array $options = []): string {
   -      watchdog_exception('Routing', $e);
   +      \Drupal::logger('Routing')->error(...Error::decodeExceptionWithMessage($e));
   

   may need DI too

3. +++ b/core/modules/system/src/SecurityAdvisories/SecurityAdvisoriesFetcher.php
   @@ -321,7 +322,7 @@ protected function doRequest(int $timeout): string {
   -        watchdog_exception('system', $exception);
   +        \Drupal::logger('system')->error(...Error::decodeExceptionWithMessage($exception));
   

   already using system logger

4. +++ b/core/modules/update/src/ProjectSecurityData.php
   @@ -224,11 +225,9 @@ private function getAdditionalSecurityCoveredMinors($security_covered_version) {
   +        \Drupal::logger('update')->error(
   

   no DI found

@andypost not sure if you meant to leave in Review for the points you mentioned.

I set NR for my changes, also fixed #163-3

Can we remove the IS update tag?

Also applied patch #168 and there were 2 instances of watchdog_exception I found that may need replacing.

In SecurityAdvisoriesFetcherTest

  /**
   * The log messages from watchdog_exception.
   *
   * @var string[]
   */
  protected $watchdogExceptionMessages = [];

Then in CronSuspendQueueDelayTest
// Capture logs to watchdog_exception().

Everything else looked good to me.

Thanks @kim.pepper!

The patch looks good, but in my opinion the change record needs to be a bit more verbose on other ways the watchdog_exception needs to be replaced:

Only the 2 first parameters of this function are covered:

function watchdog_exception($type, Exception $exception, $message = NULL, $variables = [], $severity = RfcLogLevel::ERROR, $link = NULL)

There is no example of how to use $link and other errors than RfcLogLevel::ERROR.

+++ b/core/lib/Drupal/Core/Utility/Error.php
@@ -75,6 +75,25 @@ public static function decodeException($exception) {
+  /**
+   * Decodes an exception and retrieves the correct caller with a message.
+   *
+   * @param \Exception|\Throwable $exception
+   *   The exception object that was thrown.
+   * @param string $message
+   *   A custom error message.
+   *
+   * @return array
+   *   An error in the format expected by Drupal::logger()->error().
+   */
+  public static function decodeExceptionWithMessage($exception, $message = Error::DEFAULT_ERROR_MESSAGE) {
+    $decode = static::decodeException($exception);

+++ b/core/modules/system/src/SecurityAdvisories/SecurityAdvisoriesFetcher.php
@@ -152,7 +153,7 @@ public function getSecurityAdvisories(bool $allow_outgoing_request = TRUE, int $
+        $this->logger->error('Invalid security advisory format: ' . Json::encode($advisory_data), Error::decodeException($unexpected_value_exception));

There's no cases of ever passing a $message in core so it seems this is unnecessary. Everywhere it could be used we see: $this->logger->error('Invalid security advisory format: ' . Json::encode($advisory_data), Error::decodeException($unexpected_value_exception));

So perhaps this method that returns a tuple is unnecessary.

Did we consider adding Error::logException($logger, \Exception|\Throwable $exception, string $message = Error::DEFAULT_ERROR_MESSAGE, array $additional_variables = []) :void

The implementation could then be:

$logger->error($message, static::decodeException($exception) + $additional_variables);

1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -302,7 +310,9 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
                    }
                    catch (EntityStorageException $e) {
   -                  watchdog_exception('system', $e, 'An error occurred while notifying the creation of the @name field storage definition: "@message" in %function (line %line of %file).', ['@name' => $storage_definition->getName(), '@message' => $e->getMessage()]);
   +                  $variables = Error::decodeException($e);
   +                  $variables['@name'] = $storage_definition->getName();
   +                  $this->logger->error('An error occurred while notifying the creation of the @name field storage definition: "!message" in %function (line %line of %file).', $variables);
                    }
   

   This is reverting back to a !placeholder, that doesn't exist and is wrong. Not sure why that's not failing, not tested?

2. +++ b/core/modules/system/src/SecurityAdvisories/SecurityAdvisoriesFetcher.php
   @@ -152,7 +153,7 @@ public function getSecurityAdvisories(bool $allow_outgoing_request = TRUE, int $
            // this is highly unlikely we should still display the items that are
            // in the correct format.
   -        watchdog_exception('system', $unexpected_value_exception, 'Invalid security advisory format: ' . Json::encode($advisory_data));
   +        $this->logger->error('Invalid security advisory format: ' . Json::encode($advisory_data), Error::decodeException($unexpected_value_exception));
            continue;
   

   this was not introduced here, but this usage is wrong. the message must never by dynamic. The messsage is passed to t(), every unique combination will result in a separate, translatable string and fill up the locale tables. the advisory data needs to be put in the variables and passed together with the decoded exception.

Wow quick turnaround on the new sdc module.

These ones are sometimes above my head but from what I can tell all the points 173 + 174 have been addressed. So don't mind remarking.

1. +++ b/core/includes/bootstrap.inc
   @@ -124,21 +125,37 @@ function t($string, array $args = [], array $options = []) {
   +  $variables = [];
   

   This is a breaking change - we should not be setting $variables to an empty array here.

2. +++ b/core/lib/Drupal/Core/Database/database.api.php
   @@ -202,7 +202,7 @@
   - *     watchdog_exception('type', $e);
   + *     \Drupal::logger('type')->error(...Error::decodeExceptionWithMessage($e));
   

   Let's change this to use the new Error::logException() too.

3. +++ b/core/modules/sdc/sdc.services.yml
   @@ -8,6 +8,7 @@ services:
      Drupal\sdc\Twig\TwigComponentLoader:
        arguments:
          - '@plugin.manager.sdc'
   +      - '@logger.channel.sdc'
        tags:
          - { name: twig.loader, priority: 5 }
    
   @@ -53,3 +54,8 @@ services:
   
   @@ -53,3 +54,8 @@ services:
      Drupal\sdc\Component\ComponentValidator:
        calls:
          - [setValidator, []]
   +
   +  logger.channel.sdc:
   +    class: Drupal\Core\Logger\LoggerChannel
   +    factory: [ '@logger.factory', 'get' ]
   +    arguments: [ 'sdc' ]
   

   This module will be added to the core library and will not be a module. I think we should take this opportunity to chose an existing logger channel. I think this message should end up in the default logger channel. Ie. logger.channel.default.

+++ b/core/includes/bootstrap.inc
@@ -136,9 +143,18 @@ function watchdog_exception($type, Exception $exception, $message = NULL, $varia
-  $variables += Error::decodeException($exception);
-
-  \Drupal::logger($type)->log($severity, $message, $variables);
+  $level = match ($severity) {
+    RfcLogLevel::EMERGENCY => LogLevel::EMERGENCY,
+    RfcLogLevel::ALERT => LogLevel::ALERT,
+    RfcLogLevel::CRITICAL => LogLevel::CRITICAL,
+    RfcLogLevel::WARNING => LogLevel::WARNING,
+    RfcLogLevel::NOTICE => LogLevel::NOTICE,
+    RfcLogLevel::INFO => LogLevel::INFO,
+    RfcLogLevel::DEBUG => LogLevel::DEBUG,
+    default => LogLevel::ERROR,
+  };
+
+  Error::logException(\Drupal::logger($type), $exception, $message, $variables, $level);

I was thinking about asking for a test about passing variables into the watchdog_exception() function because we broke it... and this has lead me to wondering why are we changing the deprecated code? I think in general we should only do this when we have to and I can't see a reason why we have to here.

From what I can tell #185 + #187 have been addressed. Don't mind moving this forward.

1. +++ b/core/lib/Drupal/Core/Routing/MatcherDumper.php
   @@ -51,13 +53,18 @@ class MatcherDumper implements MatcherDumperInterface {
   -  public function __construct(Connection $connection, StateInterface $state, $table = 'router') {
   +  public function __construct(Connection $connection, StateInterface $state, protected ?LoggerInterface $logger = NULL, $table = 'router') {
        $this->connection = $connection;
        $this->state = $state;
   -
   +    if ($this->logger === NULL) {
   

   This won't work how we're expecting here.

   If someone is extending it, their third param will be a string (The table name) and they will get a fatal here rather than a deprecation error.

   I think we need to remove the type hint and use an instanceof check too.

   There are four scenarios here:
   * two params
   * three params, where third is a string
   * three params, where third is a logger
   * four params

   We're missing handling for the second case

2. +++ b/core/lib/Drupal/Core/Utility/Error.php
   @@ -75,6 +77,24 @@ public static function decodeException($exception) {
   +  public static function logException(LoggerInterface $logger, \Exception | \Throwable $exception, string $message = Error::DEFAULT_ERROR_MESSAGE, array $additional_variables = [], string $level = LogLevel::ERROR): void {
   

   Exception extends Throwable, any reason we needed the union here instead of just using Throwable?

Unrelated, but I'm not too keen on the use of colinodell/psr-test-logger when we already had \Symfony\Component\Debug\BufferingLogger - I commented in #3321905: Add colinodell/psr-test-logger to core's dev dependencies to clarify why we need both.