watchdog() does not type its array arguments

An issue should be created with the offending contrib module asking them to fix the code. This is not really a core issue at all, core does not babysit broken contrib code.

This is a little odd...

A developer uses a function.. the function works... the function saves data (good or bad) to the database.

The supporting tools then read the data from the DB without caring if the data is good or bad, they just assume it was saved as 'good'.
The log reading tool then fails horribly and stops any future log messages from being seen... debugging becomes problematic... hair is lost :(

Shouldn't the tool (watchdog) that inserts the data into the DB check it for validity? And if not, then the tool that reads the data should be able survive from invalid/bad input.

This is a data validation and error handling issue... nothing to do with custom code IMHO.

The caller has to provide correct information - this is the drupal way

As a developer using random contrib modules without reading and reviewing every line of code, I would NEED the proposed 'silent fail' option to provide proper feedback to the module author/community.

If I can't access the reports page, I can't tell them what broke! The reports system is (partly) a developer debug system. It's a bit sad when your debugger dies when trying to debug something... it's like Apache throwing a segmentation fault when PHP code dies... NOT EASY to fix!

If this (as a minimum) solution isn't provided, I am more likely to re-install drupal, the DB and delete the module as it 'brings headaches' and is not worth my time. Otherwise I risk loosing 1 hour+ to debug. That 1 hour + could be spent on providing a proper bug report for the module author instead of debugging drupal itself.

Solution is raise a bug with the module developer, this really really is a won't fix. And silent failures are even worse as no one thne knows about the problem.

hmm i just ran into this issue developing my own module -- i really think it's a core issue that needs fixing!

i see marcingy's point, but you can't just allow something to break so severely because of a simple mistake in the use of a core function. Core functions should not be allowed to corrupt the database...

The function does silently fail, which is exactly the problem. You said youselves that silent failure is a bad thing, so why allow it to happen? If given a string as a 3rd argument, watchdog() does not raise any alarms, and instead silently puts the wrong type of variable into the database, which in turn blows up the "recent log entries" page.

I think there's one easy fix for this... If the wrong types of arguments are provided to watchdog(), show an error message instead of silently letting it fail, and stick a value into the database that will break other pages...

You can corrupt the database in many ways throughout drupal. This is something for modules calling the api to make sure they get correct. And if watchdog corrupts while you are testing the module truncate the table etc.

in my case i was concatenating several strings into the message area of watchdog, and accidentally typed a comma instead of a period. I really would expect to get an error at the moment i execute the code, not 10 minutes later when i pull up the recent log entries.

it sounds like there was even a module at some point that had been released with this error, because the developer obviously hadn't checked the logs during testing... why would you? if things appear to be functioning in the admin and front end, how often to you say to yourself as a developer, "i'd better make sure the logs still work" ?

Because of the lack of response to bad input within the watchdog function, there's no immediate logical connection to the error created in the log entries to the typo in the code that caused it. This is why more sophisticated languages are typed (and PHP 5 is moving toward type-hinting in function arguments). Because PHP is not typed, we should take more care in how we handle function arguments -- especially those that interact with the database... I really feel like this could be as simple a fix as 1 single line of code... It's obviously cost at least 3 developer's time...

I agree with #13/tmsimont. The scenario is like fixing the oven and not checking that the toaster still works. Everyone can test but not everyone should test everything that ever existed. Ideally 1 of 2 solutions should solve our problem. Either the log system is robust enough in it's display system to handle odd/bad data or the system of saving is more reliable and helps developers write better code. I'd opt for the latter solution. Validate the input being sent to watchdog so that life goes on less painfully and quicker than it currently does.

I think we all agree that the watchdog system is something that should never fail due to how critical it is to resolution progress.

Setting to active to help this issue find some more piece and not haunt others.

All code should be tested, drupal does not babysit broken code and no where in drupal beyond FAPI is input validated. It is a developers responsibility to provide code that meets the API standards.

1, This conversation is hilarious.

2, Seems a bit counter-intuitive to allow the error logging mechanism to break, making it more difficult to debug your module.

Not going to stoke the fire by changing the status again, just wanted to throw in my $.02

I just accidentally put a string into Drupal's core l() function and I immediately got a type error.

See Drupal's l()
http://api.drupal.org/api/drupal/includes%21common.inc/function/l/7

This is a single word fix! If Drupal doesn't babysit then why does l() type it's arguments? The same typing of watchdog's arguements would fix this... I really don't understand the opposition, it's not like it's a unique opinion. There are 4 developers posting here and 7 following, but only 1 person stone-walling what is obviously the right thing to do to address this. Why the resistance?

I've just come across this problem myself.

Sorry to add fuel to the fire but I tend to agree with cam8001 and tmisimont on this one. Allowing core functions to insert bad data into the database even if misused seems a bit obtuse and breaking the log page makes things much more difficult.

Although I do agree that fixing the misuse of the api is important for the developers and their modules, I think that not allowing the core function to insert bad data is more prominent.

marcingy, you are being a child here! can we get somebody with some sense to look at this issue?

how ironic that the function throwing the error caused by the bad data put into the DB by watchdog, drupal's core t() function has a typed argument:
http://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/t/7

the core function l() has a typed argument:
http://api.drupal.org/api/drupal/includes%21common.inc/function/l/7

why does watchdog() not have a typed argument for the $variables argument?? are these other functions not drupal core?

No I not - I am not the only person in this thread who has said it is won't fix and it is responsibility of people calling the function to do it correctly. Adding a type hint like l and t I can agree with.

I'm glad we finally agree!!! Sorry I'm getting passionate about this I just don't think you were hearing me until now.

Type hinting is all I'm asking for here -- it's in other core functions. It's the reason the reports page chokes on bad data stored by mis-used watchdog: t() gets the wrong type of argument if the wrong data type comes from the database, throwing a type error.

There should be type hinting in watchdog() just like t() and l() -- it would solve this by catching the type error at an earlier moment, and it can be done with only 6 characters of code!

cam -- t() and l() both have type hinted arguments that are optional arguments, if you need to use the function without the 3rd argument, you can safely omit it. if you need to get the 4th argument in there you can just pass an empty array as the 3rd argument?

from a cursory glance:
examples of skipping that argument without passing "null" are in core:
http://api.drupal.org/api/drupal/includes%21actions.inc/function/actions...
http://api.drupal.org/api/drupal/includes%21ajax.inc/function/ajax_get_f...

however, i see other places that will break:
http://api.drupal.org/api/drupal/authorize.php/function/authorize_access...

and of course contrib code....
so shit.. we are stuck with an un-typed function. unless every use of the null argument is updated.

not that bad.. but then there's contrib, too...

so now the question is more about if it's worth updating all of this or not.

perhaps not, but this still should probably be fixed

should it be addressed in 8.x? there are far less 8.x modules and core is still in dev so it wouldn't be as bad.

thoughts?

ok cool thanks cam8001 -- marcingy what are your thoughts on tackling this in d8?

From a d8 prospective looks good.

Committed to 8.x. Thanks!

WoW!

Just want to say that I thought this issue would explode into a flamewar but it didn't. Despite differing opinions and approaches, a common goal was found. As someone that's still kind of knew to the issue resolution process in Drupal, I'm inspired. The open source community can really help provide the dream collaboration setup we all want!

Captain Drupal would be proud.

BTW, do we backport this to D7 or would that break too much stuff?

After some local DrupalCon chat, seems like this should be back ported to D7. Updating as applicable.

Note that this is not an API change because the documentation makes it obvious that $variables should be an array.

Lets do this the same way as d8 which is based on type hinting rather than casting when building log_entry array as that is hiding error rather than providing the user with type information.

See comments in http://drupal.org/node/1279680#comment-6389230 this indicates to me that a start port of the d8 patch has been agreed as being acceptable.

The comment in #39 appears to be mistaken. API documentation for watchdog says this about $variables:

$variables: Array of variables to replace in the message on display or NULL if message is already translated or not possible to translate.

(emphasis added).

wouldn't passing NULL to t() have the same problem as passing null to a typed watchdog() function?

that is, null != array and so you'd get a recoverable fatal error.

I think the ideal fix would be to do what was done to D8 and type the argument, but if that will blow up contrib in D7, then maybe it shouldn't be backported at all.

How does the Drupal updating process handle situations like this?

There is a good solid fix in D8 that should be backported, but backporting will affect the API and cause code to break.

cam8001 has a good workaround, but should workaround be put into core?

This issue should not have the coding standards tag. It is cluttering up the coding standards issue queue. :)

Updated issue summary.

Rewrite of #41 to truly avoid the API change. Since the theme_dblog_message() function explicitly avoids calling t() when the database field contains NULL, the converse watchdog() function should allow storing NULL into the database when it is explicitly passed as a parameter.

@pillarsdotnet --

the problem with that is marcingy's initial resistance to fixing this issue at all:

drupal does not babysit broken code and no where in drupal beyond FAPI is input validated. It is a developers responsibility to provide code that meets the API standards.

Typing the argument will raise issue immediately when the function is misused, where adjusting the variable as you have done here will only silently fix a problem, which is more "babysitting" than anything.

I personally like your patch but i also agree with marcingy's original complaint that core code shouldn't have to make exceptions for misusing functions.

Are you saying that you prefer #41 to #52?

Sorry; re-uploading #41, then.

#41 does not work either as per this comment http://drupal.org/node/1279680#comment-6389230 and this http://drupal.org/node/1279680#comment-6401478 the d7 fix should work in the exact same as d8s ie it should type hinting, and should do no casting anything else hides real issue in code.

there is a fix already committed to d8 -- committing that same fix to D7 will surely break contrib modules. So is this a "won't fix" in D7? That kinda sucks, but at least it is fixed for the next Drupal version. I'm not sure what else you can really do at this point.

So I just ran into this problem. Was writing some code, and accidentally wrote a bad watchdog function.

Unfortunately now my `admin/reports/dblog` page is left in an inconsistent state. While I know to resolve this issue via purging the database of the offending rows, any common site owner who might encounter this issue will not. Their site will simply be broken, and be provided with no guidance on how to resolve this issue.

Additionally any poorly written code enabled on a site will create this irreversible problem for a common site owner.

At the very least, we should not allow dblog page to be left broken, and also provide guidance on how to avoid / resolve the issue. Also keep in mind, that `admin/reports/dblog` page is not required to be incredibly performant....as anyone who wanted performance would have this in syslog or something else.

I personally believe the watchdog should check to see if the variable is an array, and if it's not it should do nothing, and provide a warning. This would prevent bad code from getting written and also not leave dblog in an inconsistent state. I do believe that has already been discussed and disregarded though.

With that said, here's my warning patch for dblog.

This has already been discussed.

It's a bummer that the error does occur, but at least people are finding this post. Yes there could be code altered in core to prevent this from happening, and it's been addressed in 8.x

But the problem is that there's no clean fix available for 7.x that won't break existing modules and won't be a clunky additions of "babysitting" code... If you find this issue, you can see there are numerous ways to work around the problem.

To resolve this for yourself, please find the offending use of watchdog() and either fix it yourself or post issue with the module that owns the watchdog() usage.

fixed more grammar

But (I know I'm kind of switching sides on this debate),

If you were to put in the kind of fixes suggested here for this kind of function mis-usage, then we'd open the door to "babysitting" all kinds of function mis-usage. As marcingy pointed out before, you can break your site with core functions in many places.

I think the cleanest and up-to-standards fix was to type the argument of the function. I can see how other patches suggested here would address the issue, but they introduce a silent failure system that allows poorly written code to exist without consequence.

I don't think that's a good idea.

It would be nice to backport the D8 fix, but that would break way too many sites...

on the other hand... I just re-read some of my original issues:

I really would expect to get an error at the moment i execute the code, not 10 minutes later when i pull up the recent log entries.

There is an existing silent failure going on here...

What about putting in code that checks if the argument: is_array($variables) || $variables == NULL

If that fails to pass, can a PHP error be thrown? Although it's kind of "babysitting," in this case it's more of a backport than it would be a babysit... The result would be a fix with functionality that matches the D8 commit, without breaking existing contrib modules that expect NULL to be an OK parameter.

(active not "needs review" -- "needs review" insinuates a patch)

Well "bad calls" should be broken. What we don't want to break is what were previously considered "good calls" that are now "bad"

By typing the argument as an array, passing "NULL" is now a "bad call"

Basically, to not break the previously "good calls" and to stop all "bad calls," we need to type the argument as an array while allowing NULL to also pass. That's not possible with type-casting in function signatures, so maybe this code in the function would work:

if (!is_array($variables) &&  $variables != NULL) {
    trigger_error("Invalid argument passed to watchdog()", E_USER_ERROR);
}

I'm not sure if trigger_error is really a Drupaly function tho.

@tmsimont the idea of throwing a php error if the value is not the correct data type seems like it could be compromise option for drupal 7 as the code it adds is minimal and it does not mask errors.

Also it would mean:

a) No broken data in the table
b) The developer could track down the error and fix the underlying issue.

The main thing that concerns me is that it could set a precedence for functions checking incoming data.

I agree with your concern.

I just went back to php.net and reviewed this type hinting business and we may be wrong about the type-hinting being a dealbreaker for null arguments:

http://php.net/manual/en/language.oop5.typehinting.php

Last example says:

Type hinting allowing NULL value:

/* Accepting NULL value */
function test(stdClass $obj = NULL) {

}

test(NULL);
test(new stdClass);

I'm not sure if that works for arrays, I thought we tried this already... let me see..

well shoot looks like this is possible..

I just ran a few tests in a drupal site and this function signature does not require API changes:

-function watchdog($type, $message, $variables = array(), $severity = WATCHDOG_NOTICE, $link = NULL) {
+function watchdog($type, $message, array $variables = NULL, $severity = WATCHDOG_NOTICE, $link = NULL) {

This would affect the committed D8 patch, too..

We were operating previously under the assumption that typing the argument required an API change, which is why backporting has become so muddy.

But, based on this info from php.net, that's not true...

attached is a patch for 8.x that undoes most of what cam8001's patch did before, and adjusts watchdog()'s function signature to work with a typed array or NULL. If I'm right, this should pass testbot and we can backport this directly

yay! green. if we can get that committed to 8.x i will backport the 7.x fix

Looks good, nice work on tracking down a solution that works for d7 and d8!

Yes type hinting an array does allow NULL to be passed. Patch looks fine to me, so committed/pushed to 8.x.

Moving to 7.x for backport.

patch attach

Looks good

If they pass strings are 3rd argument they are broken to start with so this puts us back to won't fix. This patch is all about making sure people know that the 3rd argument is an array or NULL which are infact the only 2 valid values at the moment.

@cam8001 -- breaking sites that pass strings is the point. We want to break incorrect usage of the function at the moment the function is misused. Currently, the strings pass through the function, get into the DB and break the reports page when poorly created watchdog reports are retrieved. So this will only break modules that are already broken, but it will make the break happen at a more logical point in the process.

added note about wont' fix for D7 and posted detail about D8 commit

Why would you not want to fix this?

To quote you:

perhaps the call to watchdog() should raise an error instead of silently failing and causing an error somewhere else

I think that's the cleanest, simplest and by far best way to address this. I'm confused about why you are against this.

Aren't those sites already broken?

The original issue here is the fatal error that comes of such mis-use of watchdog()

I suppose there could be live sites out there with broken logs and the developers that launched them never use the logs...

I'm not sure if it would make sense to protect these mis-used half-broken sites at the expense of allowing this error to continue to exist for future module development.

I'm not sure what to do here -- I'd vote to fix this problem but if the policy is to safeguard broken sites from being more broken, i suppose this is indeed a "won't fix."

I think a fatal error which only occurs at admin/reports/dblog is a lot less serious than one which could (potentially) occur on any page view anywhere on the site, so we shouldn't change that behavior for Drupal 7 because it could really break things.

But some of the other solutions discussed above (essentially, making sure we log something which helps identify where the real problem was, i.e. identifies which watchdog() call was to blame) seem like they'd be OK.

Honestly, I think theme_dblog_message() is the place to do that (or at least one such place)? It's the one calling t() with untrusted/unknown data, so isn't it the one which should ensure that the data matches what t() expects? I agree it shouldn't hide the error if there is one, but that doesn't mean it shouldn't protect against fatal errors either.

Nah, rereading this issue, I guess my suggestion would be a pain actually.

I think we can fix it by triggering an error in watchdog() directly though, like was suggested earlier. It's not ideal but for Drupal 7 only it seems like the best we can do and I think it would be acceptable. (In any case, it's the closest we can get to the Drupal 8 patch without breaking things.)

So... something like the attached?

Won't this have a determental performance impact?

You mean the is_array() check?

Hopefully that's pretty tiny compared to all the other things that already happen whenever watchdog() is called.

Yes I mean is_array(), well watchdog can be used for simple logging eg creating a node does so, so it is a critical path in some ways which is why I tagged this with needs benchmarks. And to be honest this is something that should be tested on a dev site anyway before hitting production so this baby sitting code just feels wrong.

So why not to prevent further execution of watchdog code? Write system's message instead of that broken and just return.

If you're going to a watchdog to watchdog bad watchdog calls, then you need to exit out of the function after the error.

Exiting out would have no more performance concerns at that point, if the watchdog was called successfully and was added to DB anyways.

With that said, dblog will kill sites and is a very large performance burden in general (many db inserts during page requests). Which is why we have syslog to replace it, but unfortunately, there's no admin interface for checking out syslog logs in Drupal admin interface, which I believe their should be (but that's another ticket).

Changing to needs work for the point I just made.

I disagree; I don't think the site owner should be punished for the sins of the module :) We should still record the original log message for them (with the original category and severity) just like before.

As for profiling, if someone wants to test the performance of this, definitely feel free, but I personally don't see the point.

is_array() is a lightning-fast PHP function. In my opinion, if your site is calling watchdog() frequently enough that you care about micro-optimizing it, you have way way way bigger problems on your hands already :)

***

Exiting out would have no more performance concerns at that point, if the watchdog was called successfully and was added to DB anyways.

Well, the performance concern raised above was specifically about the is_array() check on all watchdog calls, not about the double-logging in the case where there's an error. When we discover new errors I don't think we worry about the impact associated with logging those, because errors should be logged, and the point of doing that is so they can be found and fixed anyway...

I don't believe the is_array() is going to be a performance issue. Largest performance issue with watchdog is dblog doing an insert.

Unfortunately double insert's if you want to keep the original error message, there an additional performance burden.

If you look at the patch you'll notice the original message is appended to the "this watchdog call is bad" watchdog. Personally I think that's the best route. Module writers should notice that their watchdog calls are not showing up, and hopefully dig into the issue and resolve the problem. If they did show up in dblog, they might overlook the "your watchdog is bad" calls.

It's debatable what's best, but double inserts of bad watchdog calls does have a performance burden, so I'm in favour of how the patch currently works with a single call and exiting out.

I have seen sites die from watchdog & dblog combination in the past and usually disable dblog in favor of syslog or nothing because of this on sites where performance is required.

But you wrote the patch, so do what you will :) Either way I'll be happy that a bad watchdog call will not kill the admin interface.

If you look at the patch you'll notice the original message is appended to the "this watchdog call is bad" watchdog. Personally I think that's the best route. Module writers should notice that their watchdog calls are not showing up, and hopefully dig into the issue and resolve the problem. If they did show up in dblog, they might overlook the "your watchdog is bad" calls.

I totally agree with j0rd -- double-entry of the bad call would be a performance hit, but more than that it's just not necessary of the "this is bad" message has the original text.

It's not just about the message text, but also about the severity (i.e. "error" vs. something else) and also the type. For example, if you are filtering for messages provided by a particular module (very common on sites which have large watchdog logs) you would miss it embedded within this other message because it shows up in a totally different place.

IMO that's still an improvement from the current behavior, which is total destruction of the logs page...

Agreed, but the patch I posted in #91 is intended to handle both :)

i know, but it comes at the cost of a double database insert. i guess it doesn't really matter if you're not concerned about performance of a mis-used function, but a developer that is misusing the function might not ever notice that there are 2 logs per function call... it seems to open up a door to unchecked bad practice. the patch committed to 8.x just throws an immediate error when the function is mishandled and everything stops before any DB writes occur. i'd think we'd want to stop mis-use of the function in 7.x as well, in a manner that tells the developer that he is doing wrong, rather than allow it to quietly be mis-used with a double database write.

but that's all a bunch of opinion... in reality either #91 or #103 would work. i dont know which is really better since in both cases we're dealing with an already mis-used function. i dont think there's a standard for this kind of thing, which is why marcingy was originally so adamant about keeping this as "won't fix"

see #41 through #58...

also see #79 through #89

Hi, unfutunetlly I am having the same error when I try to see my logs.
I also cant [And I try] understand much of writen here and trying to figure out what can I do now since I can't even see the cause of the error.
My error:

Recoverable fatal error: Argument 2 passed to t() must be an array, integer given, called in /home/pukka/public_html/modules/dblog/dblog.admin.inc on line 295 and defined in t() (line 1452 of /home/pukka/public_html/includes/bootstrap.inc).

Please leave version alone on issues

To summarise, this bug happens when modules incorrectly call watchdog, and has been fixed in drupal 8 by using type hinting to trigger a PHP fatal error when the incorrect call is made. For Drupal 7, we have five options:

1) Won't fix
Advantages: Easy. Nothing is worse than now.
Disadvantages: A site builder may find a Drupal Core feature crashing on them, with no obvious hint as to what caused the issue or how to fix it.

2) Backport the Drupal 8 fix
Advantages: The most technically elegant. Fixes the problem really well for new contrib code.
Disadvantages: A site owner may find that upgrading to the new version of Drupal Core means they have gone from a site with a broken log page, to a site with a broken homepage.

3) Silently fix the error
Advantages: Site builders get all the error logs the module author intended. Miniscule performance impact.
Disadvantages: Encourages module authors to write incorrect calls, which will cause the original bug for people who are running older versions of Drupal. Inconsistent with Drupal 8 behaviour.

4) Fix the error and log a warning (patch #91)
Advantages: Site builders get all the error logs the module author intended. Incorrect call should be spotted by the module author or more technical module users and fixed in due course.
Disadvantages: Small performance impact and clutters up the logs.

5) Replace the error (patch #103)
Advantages: Nothing breaks. Miniscule performance impact.
Disadvantages: Site builders won't see the error that the module author was trying to log.

4b/5b) These patches could be tweaked to log the additional error message with the same type as the original error message, to name and shame the offending module when filters are in place.

Are there any other alternatives? Which of these is the best?

I'd say option 4 is the most helpful to all concerned and doesn't have any serious drawbacks, closely followed by 5 then 3. Options 1 and 2 are by far the worst as they cause functionality to be totally broken.

I can't see any disadvanges to option 5 when compared to option 1, so if people have concerns about the other options then we should get option 5 committed so we've fixed the major problem, and open a followup to discuss the bells and whistles.

This bug sounds important to me.

This is not a major bug as per https://drupal.org/node/45111

That issue broke my development database.

103: watchdog-type_hint_array_arguments-1279680-103.patch queued for re-testing.

That this issue is still broken after three or more years is rather hard to believe... any chance of movement?

Posting a patch for people needing to access broken logs. The patch to review/commit is still #103 or #91.

91: watchdog-type_hint_array_arguments-1279680-91.patch queued for re-testing.

Thanks, plach, for the temporary patch in #118 so that I could view my (broken) logs. I then patched bootstrap.inc with #103, and I'm able to now see where exactly the issue was triggered.

How is this not Major? If you run into this, you have to hack core (#118 thanks!) or not have access to the dblog at all? That sounds pretty major. Feel free to explain the nuance of why not again, but I hope this will be fixed. I know webform happens to be the cause of the problem in my case (https://www.drupal.org/node/2542796) but still...

I think the reasoning is that not passing an array is a bug in the calling code, not in watchdog() (motto: "don't babysit broken code").

#118 worked for me, thanks!

@fgm This is the same type of thinking that would get people killed in car accidents if car manufacturers stopped making seatbelts simply because it's not needed if there is an airbag...... Just because someone doesnt use a seatbelt (thinking airbag is enough) doesnt mean the seltbealt should not exist. "Baby sitting bad code" is not the only reason that bad data could be injected into the db. There are such things as DB corruption (the server died for example) that could very well inject bad data. Therefore it's absolutely imperative that checking is done on BOTH ends. Not just one end. Everyone in this thread complaining that this should be fixed is 100% correct. It needs to be fixed on the read and #118 has this in 2 measly lines. It needs to be implemented, not dragged out for years on end based on il-informed principles. "Do not stick one's head in the sand due to lazyness" is a better motto. Along with #118, it would be nice if it also notified the user that bad data was detected, where, etc so that the offending code (if any at all) could be fixed.

@crystaldawn : my comment is not meant to claim this didn't need to be done, just explaining why it had not been done because of the existing core policy, which has been used over the years to Won't Fix several patch suggestions I did in much the same vein, based on the old internet adage (be [...] tolerant in what you receive) advocated here.

It's interesting to see the mentalities shift over time, though, to a safer one.

Just for the sake of completeness, I see nobody has described to newbies how to see their logs when this breaks... here's a simple drush query for you to see the last 50 dblog records.

drush sql-query "SELECT * FROM watchdog ORDER BY timestamp DESC LIMIT 50"

Have a look at the 5th column... Most of them will start with `a:` which indicates a serialised array. If you see a row where the fifth column starts with `i:` (integer) or `s:` (string), then that's the one causing the error. Hopefully, the third column of that row will tell you which module has the bad code.

Happy hunting!

To add to @Chris Gillis (#133), I ran into this while developing my own module. If you want to remove the bad logs while keeping the rest of your logs in tact, this is what I used:

sql to inspect offending watchdog entries:

select * from watchdog where variables <> "N;" and variables not like "a:%";

sql to remove offending watchdog entries:
delete from watchdog where variables <> "N;" and variables not like "a:%";

My issue came down to accidentally omitting the $variables parameter, so $variables was being set to WATCHDOG_INFO (integer 6).

D7 patch to fix a problem when there are already corrupted data in the watchdog table has been fixed today in 7.90 (#2790857: Log completely unusable when an entry has corrupt serialized data (D7)), so this issue can focus on backporting the solution already commited here to D8 (typecasting), if possible.

Leaving as Needs review - patches for D7 are here in #91 (or #103).

Thanks all for working on this. I think we should close the D7 attempt as Won't fix. The reason is, that we cannot introduce a strict agrument typing, as it can cause fatal errors on sites (see #90). On the other hand, the patches in #91 and #103 are doing the similar thing that was already committed in #2790857: Log completely unusable when an entry has corrupt serialized data (D7). So I think we do not need to add this protection again. The protection on output should be sufficient and we should allow to write even if it is not an array. We are also printing a message on output, if the variables are malformed, so I do not see anything else we can do here for D7.

Closing this as Fixed for D8, so that credits get assigned correctly.