Warning: htmlspecialchars() ... on cron runs when an update is available

[indigoxela]

Description of the bug

When an update is available, whenever Backdrop checks for them on my sites (according to the update interval setting (daily / weekly...)) via cron run, a php warning appears twice in dblog:

Warning: htmlspecialchars() expects parameter 1 to be string, array given in check_plain() (... 2054 ...core/includes/bootstrap.inc).

Steps To Reproduce

That's a bit tricky, as this only appears when updates are available. Not if there aren't any and not if running the update check via UI. Also, it might be necessary to install an additional language. Note: the sites aren't multilingual, it's just that the default language isn't English.

Additional information

This warning isn't new, I'm not even sure, if it hasn't been there ever since I'm using Backdrop. At least I don't remember when I first saw that in dblog.

This time I was smart and put a debug($text) in bootstrap.inc of a dev site. This is what check_plain() gets as array then (for the two notices):

1. array()
2. array('langcode' => 'en',)

[indigoxela]

The problem is the t() call for the mail subject in function update_mail().

The code is syntactically correct but still wrong.

https://github.com/backdrop/backdrop/blob/557824453eeec969800c2ff47db7bde4da036467/core/modules/update/update.module#L448

Which is easier to see, if trying to apply some indentation:

  $message['subject'] .= t('New release(s) available for !site_name', array(
    '!site_name' => config_get_translated('system.core', 'site_name'),
       array(), array('langcode' => $langcode)), array('langcode' => $langcode));

A PR is available, but testing can't happen in the sandbox, it needs a bit more this time.

How to test:

1. Get / install an outdated Backdrop version
2. Apply below sql to its db table "state"
3. Run cron via command line (see the php warning)
4. Apply the patch
5. Apply below sql again
6. Run cron via command line (no warning this time)

sql:

UPDATE state SET value = 'i:1589968800;' WHERE name = 'update_last_check';
UPDATE state SET value = 'i:1589968800;' WHERE name = 'update_last_email_notification';

Sending the mail is not affected by this problem, btw, but make sure that sending mails on available updates is turned on to trigger function update_mail().

[indigoxela]

Update: I had to fix my own wrong usage... Now it's really correct and (hopefully) readability has improved.

[ghost]

This happens on a fresh install using the Backdrop Tugboat demos:

(the screenshot's purpose is to show that this warning appears on a fresh install (i.e. Standard module just installed), the other PHP warnings/notices are Tugboat related).

[indigoxela]

@BWPanda what are the details of this log message?

[ghost]

Same as yours: Warning: htmlspecialchars() expects parameter 1 to be string, array given in check_plain() (line 2054 of /var/lib/tugboat/core/includes/bootstrap.inc).

[indigoxela]

Same as yours

Sure, but triggered by what? check_plain() is used all over the place... 😉

Thinking this over - the dblog message detail won't help anyway, as it only contains "Location" as a not so helpful hint. Hm... and probably you can't patch around in a Tugboat instance.

It might or might not be related to this issue - we won't be able to find that out, I guess.

[ghost]

I actually discovered this issue when testing my custom install profile (installing the same test site over and over again). I noticed these two warnings in the log and came searching to see if others had had them too. To my surprise this issue was only hours old 😄

Since I didn't know if my custom profile was causing it, I tested a fresh, core Backdrop install on B.org's 'demo' page, and posted the screenshot here. Will see if I can work out the cause...

[ghost]

Think I found it. Using Xdebug I found where this is getting called:

check_plain() <- backdrop_placeholder() <- format_string() <- t() <- update_mail() <- ...

The problem seems to be with this line in update_mail():

$message['subject'] .= t('New release(s) available for !site_name', array('!site_name' => config_get_translated('system.core', 'site_name'), array(), array('langcode' => $langcode)), array('langcode' => $langcode));

Here's the call to t() with line breaks and indentation to make it easier to read:

t(
  'New release(s) available for !site_name',
  array(
    '!site_name' => config_get_translated('system.core', 'site_name'),
    array(),
    array('langcode' => $langcode)
  ),
  array('langcode' => $langcode)
);

So that's where the two arrays are coming from. This seems to have been introduced in backdrop/backdrop@32a8aa9 and it looks like it's just a ) in the wrong place...

[indigoxela]

Ehm... I'm confused... Did you miss that a PR already exists?

And the existing PR fixes exactly that function...

Or did you try to figure out the problem with the Tugboat instance? If it's the same problem, then this PR also fixes that.

[indigoxela]

Yeah, I guess you totally missed my PR... And didn't read my comments.

[ghost]

Did you miss that a PR already exists?

Doh, yes 😦 Ignore me, I'll just go hide in the corner now...

[indigoxela]

I'll just go hide in the corner now...

Or even better - you could do testing and/or code review. 🤣

[ghost]

Small change requested for PR.

[indigoxela]

@BWPanda welcome back from the corner 😸 and many thanks for reviewing.

The PR has been updated accordingly.

[ghost]

Thanks for the change. Code looks good and fixes the issue as I was experiencing it. Don't want to commit based just on my review, so will need another RTBC before I commit the PR.

[klonos]

Oh, good job both @indigoxela and @BWPanda 👍 ...just by looking at that function, I thought that the entire , array('langcode' => $langcode)) at the end was an accidental additional copy. I had to double-check the definition of both t() and config_get_translated() on api.b.org to realize what's going on 😅 (they are similar in that they both accept $args = array(), $options = array() as additional optional parameters)

Anyway, code looks good now, and in fact thank you @indigoxela for breaking that function up. It's much more readable and less likely to cause confusion now 👍

[ghost]

Thanks for this @indigoxela! Merged backdrop/backdrop#3153 into 1.x and 1.16.x.