Deprecate NULL values in Html::escape(), ::decodeEntities(), and FormattableMarkup::placeholderFormat() to make it easier to upgrade to PHP 8

I face this issue when working with search_api_solr reindexing + PHP 8.1.

MR created for 9.4.x-dev

Rebased. No functional change except that if NULL is passed to placeholderEscape(), Html::escape() will get empty string instead of NULL. Includes test.

Patch review, this should avoid the call to escape on null and just return the empty string.

Regarding the approach, I'm not sure this is correct. Should we allow null? The documentation on the method is very explicit
"a string or an object that implements \Drupal\Component\Render\MarkupInterface" and the examples go into quite a bit of detail to show how to make it a safe string or Markup.

Additionally, something passing null is probably hitting an unrealized bug. That's kinda the reason PHP deprecated these null string conversions.

Side note: pretty sure deprecation notices aren't major, just normal. Users shouldn't see them since we expect deprecation errors to be off except for development and testing.

Would a suitable solution be to check the type of $value and if the type is wrong, run something like the following?

trigger_error('Html::escape(): Passing null to parameter #1 is deprecated.', E_USER_DEPRECATED);

Could be. It feels weird adding a deprecation... to a deprecation? Maybe that's the correct approach to set our own timeline though?

For now, perhaps it's fine as it is and contrib needs to be updated to not send NULL to this function.

Once we require PHP 8.1, we can add a type declaration using union types. That would make it easier to find where this is being called with the wrong type.

Perhaps this should be included in a broader issue about adding type declarations.

I get this error

Deprecated function: htmlspecialchars(): Passing null to parameter #1 ($string) of type string is deprecated in Drupal\Component\Utility\Html::escape() (line 424 of core/lib/Drupal/Component/Utility/Html.php).

Fix for issue that happens on Drupal 9.3 with PHP 8.1 :
Deprecated: htmlspecialchars(): Passing null to parameter #1($string)of type string is deprecated in core/lib/Drupal/Component/Utility/Html.php

16# solved the problem.... I just replaced the lines in that file manually. Will I have to redo it again after upgrading to newer version of core? Will the file get overwritten?

Yes, @rossidrup. You will have to redo it after upgrading. But, manually editing core/contrib module code is not the right way.
The recommended way is to use cweagans/composer-patches and applying patch through composer. You can checkout this doc for more info: https://www.drupal.org/docs/develop/using-composer/using-composer-to-install-drupal-and-manage-dependencies#patches.

Thanks!

I see, but why does this error happen and why do they don't they hard code the patch into the next release? Does it happen only on some sites? What is the reason? I do not understand...

I could only see this error when I went to my adaptive theme settings, on bartik the error does not show up....
also it does not show up on other drupal installations that use same adaptive theme and there is no error...

This new merge request adds the type declarations.

This is a similar issue for Drupal 9.3. It does not depend on union types, so it could go in right away. Review would be appreciated.

Adding the type declarations seems like the correct way to go, however it will also change this issue from a warning to TypeError.

Stack trace:

TypeError: Drupal\Component\Render\FormattableMarkup::placeholderEscape(): Argument #1 ($value) must be of type Drupal\Component\Render\MarkupInterface|string, null given, called in /app/web/core/lib/Drupal/Component/Render/FormattableMarkup.php on line 208 in Drupal\Component\Render\FormattableMarkup::placeholderEscape() (line 261 of core/lib/Drupal/Component/Render/FormattableMarkup.php).

Drupal\Component\Render\FormattableMarkup::placeholderEscape(NULL) (Line: 208)
Drupal\Component\Render\FormattableMarkup::placeholderFormat('Create @bundle (@entity_type)', Array) (Line: 195)
Drupal\Core\StringTranslation\TranslatableMarkup->render() (Line: 15)
Drupal\Core\StringTranslation\TranslatableMarkup->__toString()
strip_tags(Object) (Line: 1372)
template_preprocess_html(Array, 'html', Array) (Line: 287)
Drupal\Core\Theme\ThemeManager->render('html', Array) (Line: 422)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 201)
Drupal\Core\Render\Renderer->render(Array) (Line: 162)
Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}() (Line: 564)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 163)
Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse(Array, Object, Object) (Line: 90)
Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object, 'kernel.view', Object)
call_user_func(Array, Object, 'kernel.view', Object) (Line: 142)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object, 'kernel.view') (Line: 163)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 80)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 708)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

So for now the solution seems easier use to #16 or MR 1659

@#23: Yes, I agree that type declarations are the best solution.

Perhaps in FormattableMarkup.php on line 208 and 232, where placeholderEscape() is called, it should check for NULL at that point or cast $value to string.

Then based upon MR 2040, this patch checks for null going into placeholderEscape()

Re-roll to correct full paths, for running tests.

@3li: Thanks. You could also click on "get push access" and then push your addition to the existing merge request.

?? will work. It could also be ?:.

Maybe it would be better if it was:

$args[$key] = $value ? ('<em class="placeholder">' . static::placeholderEscape($value) . '</em>') : NULL;

This way there is never an empty em element.

using #26 I get this from the LinkGenerator

TypeError: Drupal\Component\Utility\Html::escape(): Argument #1 ($text) must be of type string, null given, called in /var/www/html/web/core/lib/Drupal/Core/Utility/LinkGenerator.php on line 205 in Drupal\Component\Utility\Html::escape() (line 423 of core/lib/Drupal/Component/Utility/Html.php).

If I remove the type declaration it works

-  public static function escape(string $text): string {
+ public static function escape($text) {

It sounds like you are not running PHP 8.1. That is why the same error does not come up when NULL is passed to htmlspecialchars(). The purpose of the type declaration is to identify places where NULL is sent to Html::escape() so that those can be fixed.

Patch based upon the current MR.
Have added a checks to prevent passing null to escape while generating URL like in #28.

+++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
@@ -205,7 +205,7 @@ protected static function placeholderFormat($string, array $args) {
+          $args[$key] = static::placeholderEscape($value ?: '');

#23 was mistaken about ?: and its causing a _lot_ of failures. For example it is failing by converting 0 into an empty string in the bytes test. We specifically want to null coalesce and only act on null values, not check the truthyness of the input.

Yes, I see. This also not right, since it would return empty string if $value is 0.

$value ? '<em class="placeholder">' . static::placeholderEscape($value) . '</em>' : '';

It should be:

isset($value) ? '<em class="placeholder">' . static::placeholderEscape($value) . '</em>' : '';

Made changes as per comment #33.

Updated MR - tests are now passing.

"Merge blocked: the source branch must be rebased onto the target branch." tried to rebase the issue branch

Thanks guys, works, no more errors now . D 9.3.15, PHP 81.3 , 10.5.15-MariaDB.

Hi All,

I have a similar issue when indexing search, but in my case it passing an array:

Unable to decode output into JSON: Syntax error

TypeError: htmlspecialchars(): Argument #1 ($string) must be of type string, array given in htmlspecialchars() (line 424 of /var/www/html/core/lib/Drupal/Component/Util
ity/Html.php).

My stack is Drupal 9.3.15, PHP 8.1 and MariaDB 10.3.34

Do you have any suggestions to fix it?
I'm upgrading it from Drupal 9.2.20 and php 7.4

Thank you

here is traceback:

[notice] Successfully indexed 200 items.
> [error] TypeError: htmlspecialchars(): Argument #1 ($string) must be of type string, array given in htmlspecialchars() (line 424 of /var/www/html/core/lib/Drupal/Component/Utility/Html.php) #0 /var/www/html/core/lib/Drupal/Component/Utility/Html.php(424): htmlspecialchars(Array, 11, 'UTF-8')
> #1 /var/www/html/core/lib/Drupal/Component/Render/FormattableMarkup.php(262): Drupal\Component\Utility\Html::escape(Array)
> #2 /var/www/html/core/lib/Drupal/Component/Render/FormattableMarkup.php(208): Drupal\Component\Render\FormattableMarkup::placeholderEscape(Array)
> #3 /var/www/html/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php(195): Drupal\Component\Render\FormattableMarkup::placeholderFormat('The @label incl...', Array)
> #4 /var/www/html/core/lib/Drupal/Component/Utility/ToStringTrait.php(15): Drupal\Core\StringTranslation\TranslatableMarkup->render()
> #5 /var/www/html/sites/default/files/php/twig/62a0af5f99670_node--lab-group-page.html_NJrTVpSQIDUt7AmRH1cv3FRn2/t5tg2ZVR5avznBai1Sb-8ABr1VxbM356u3euJzpna3s.php(54): Drupal\Core\StringTranslation\TranslatableMarkup->__toString()
> #6 /var/www/html/vendor/twig/twig/src/Template.php(405): __TwigTemplate_9f4a9d3527f562c42477e38fb4feb281->doDisplay(Array, Array)
> #7 /var/www/html/vendor/twig/twig/src/Template.php(378): Twig\Template->displayWithErrorHandling(Array, Array)
> #8 /var/www/html/vendor/twig/twig/src/Template.php(390): Twig\Template->display(Array)
> #9 /var/www/html/core/themes/engines/twig/twig.engine(55): Twig\Template->render(Array)
> #10 /var/www/html/core/lib/Drupal/Core/Theme/ThemeManager.php(384): twig_render_template('themes/custom/n...', Array)
> #11 /var/www/html/core/lib/Drupal/Core/Render/Renderer.php(422): Drupal\Core\Theme\ThemeManager->render('node', Array)
> #12 /var/www/html/core/lib/Drupal/Core/Render/Renderer.php(201): Drupal\Core\Render\Renderer->doRender(Array, true)
> #13 /var/www/html/core/lib/Drupal/Core/Render/Renderer.php(157): Drupal\Core\Render\Renderer->render(Array, true)
> #14 /var/www/html/core/lib/Drupal/Core/Render/Renderer.php(564): Drupal\Core\Render\Renderer->Drupal\Core\Render\{closure}()
> #15 /var/www/html/core/lib/Drupal/Core/Render/Renderer.php(158): Drupal\Core\Render\Renderer->executeInRenderContext(Object(Drupal\Core\Render\RenderContext), Object(Closure))
> #16 /var/www/html/modules/contrib/search_api/src/Plugin/search_api/processor/RenderedItem.php(283): Drupal\Core\Render\Renderer->renderPlain(Array)
> #17 /var/www/html/modules/contrib/search_api/src/Item/Item.php(282): Drupal\search_api\Plugin\search_api\processor\RenderedItem->addFieldValues(Object(Drupal\search_api\Item\Item))
> #18 /var/www/html/modules/contrib/search_api/src/Entity/Index.php(976): Drupal\search_api\Item\Item->getFields()
> #19 /var/www/html/modules/contrib/search_api/src/Entity/Index.php(930): Drupal\search_api\Entity\Index->indexSpecificItems(Array)
> #20 /var/www/html/modules/contrib/search_api/src/IndexBatchHelper.php(154): Drupal\search_api\Entity\Index->indexItems(50)
> #21 /var/www/html/vendor/drush/drush/includes/batch.inc(256): Drupal\search_api\IndexBatchHelper::process(Object(Drupal\search_api\Entity\Index), 50, 2471, Object(DrushBatchContext))
> #22 /var/www/html/vendor/drush/drush/includes/batch.inc(201): _drush_batch_worker()
> #23 /var/www/html/vendor/drush/drush/includes/batch.inc(95): _drush_batch_command('1018')
> #24 /var/www/html/vendor/drush/drush/src/Drupal/Commands/core/BatchCommands.php(20): drush_batch_command('1018')
> #25 [internal function]: Drush\Drupal\Commands\core\BatchCommands->process('1018', Array)
> #26 /var/www/html/vendor/consolidation/annotated-command/src/CommandProcessor.php(257): call_user_func_array(Array, Array)
> #27 /var/www/html/vendor/consolidation/annotated-command/src/CommandProcessor.php(212): Consolidation\AnnotatedCommand\CommandProcessor->runCommandCallback(Array, Object(Consolidation\AnnotatedCommand\CommandData))
> #28 /var/www/html/vendor/consolidation/annotated-command/src/CommandProcessor.php(176): Consolidation\AnnotatedCommand\CommandProcessor->validateRunAndAlter(Array, Array, Object(Consolidation\AnnotatedCommand\CommandData))
> #29 /var/www/html/vendor/consolidation/annotated-command/src/AnnotatedCommand.php(350): Consolidation\AnnotatedCommand\CommandProcessor->process(Object(Symfony\Component\Console\Output\ConsoleOutput), Array, Array, Object(Consolidation\AnnotatedCommand\CommandData))
> #30 /var/www/html/vendor/symfony/console/Command/Command.php(255): Consolidation\AnnotatedCommand\AnnotatedCommand->execute(Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
> #31 /var/www/html/vendor/symfony/console/Application.php(1027): Symfony\Component\Console\Command\Command->run(Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
> #32 /var/www/html/vendor/symfony/console/Application.php(273): Symfony\Component\Console\Application->doRunCommand(Object(Consolidation\AnnotatedCommand\AnnotatedCommand), Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
> #33 /var/www/html/vendor/symfony/console/Application.php(149): Symfony\Component\Console\Application->doRun(Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
> #34 /var/www/html/vendor/drush/drush/src/Runtime/Runtime.php(121): Symfony\Component\Console\Application->run(Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
> #35 /var/www/html/vendor/drush/drush/src/Runtime/Runtime.php(51): Drush\Runtime\Runtime->doRun(Array, Object(Symfony\Component\Console\Output\ConsoleOutput))
> #36 /var/www/html/vendor/drush/drush/drush.php(72): Drush\Runtime\Runtime->run(Array)
> #37 /var/www/html/vendor/drush/drush/drush(4): require('/var/www/html/v...')
> #38 /var/www/html/vendor/bin/drush(117): include('/var/www/html/v...')
> #39 {main}.

@buuza have you tried to apply any of the patches?
Though it is also possible this issue is coming from search_api based upon your stack trace.

Hi @3li,
Yes, I did apply patch #16 which fixed errors and search index is working again.
Also, we found the issue with Argument #1 ($string) must be of type string, array given in htmlspecialchars() - it was twig templates passing arrays.
thanks

I tried patch -34 but this has a bug:

Uncaught TypeError: Argument 1 passed to Drupal\Component\Render\FormattableMarkup::placeholderEscape() must implement interface Drupal\Component\Render\MarkupInterface, string given, called in /var/www/html/web/core/lib/Drupal/Component/Render/FormattableMarkup.php on line 232 and defined in /var/www/html/web/core/lib/Drupal/Component/Render/FormattableMarkup.php:262

The problem is that placeholderEscape is defined as accepting *only* a MarkupInterface,

protected static function placeholderEscape(MarkupInterface $value): string {

but it's param doc:

* @param string|\Drupal\Component\Render\MarkupInterface $value

and its code:

return $value instanceof MarkupInterface ? (string) $value : Html::escape($value);

..expect that it can be passed a string value, which is what happened for my error.

The right fix for now is I think to remove MarkupInterface from the type spec, because IIRC the php version that supports variant-type parameters is newer than the minimum for D9.

This is a patch in which the one change mentioned above has been made.

@rivimey can you try to use the merge request from this issue ?

This can use union types for type declarations. Example:

protected static function placeholderEscape(string|MarkupInterface $value): string {

Patch at #16 works for me too.

Is this targetting 10.0.x-dev only because bugs get squashed for Drupal 10 first before being backported to Drupal 9? I'm simply running 9.4.3 in production and hit this when I updated to use PHP 8.1 ready for Drupal 10 (when PHP 8.0 support will be dropped).

@martijn-de-wit just to confirm, yes I did manage to apply the merge patch and it's ok now with php8.

The backport to D9 will need to remove the union types... current D9.4's composer requires { "php": ">=7.3.0" }" whereas php 8 is required for union types.

Why does the simpler approach in the patch at #16 not work correctly? It bypasses any need to use union types, and so gives a solution that works for all currently supported versions of Drupal core.

The fix in #16 would work, but it doesn't solve the underlying problem of code that sends non-strings into this function. This issue is against Drupal 10, which only runs on versions of PHP that support union types anyway.

Liam, IMO it would simplify a lot of code if this function were to accept null as well as string and markup objects. I agree it shouldn't be accepting other things (int, for example?). Return value for input=null should IMO be "".

This is covered in comment #9. Perhaps a maintainer needs to make a decision. It either needs a type declaration which matches the documentation or appropriate casting inside the function.

This is the same basic question as #3271517: Add type declarations to Html::decodeEntities() and Html::escape().

Liam, if this issue is specifically related to Drupal 10's support of union types, does that mean that I should open a fresh issue if I'm seeing the same error on a Drupal 9 site using PHP 8.1?

Usually issues are fixed in the development version and then backported. In this case, the best fix uses union types, so the fix would likely be different for version before Drupal 10. So, making another issue would be a reasonable step.

On #3271517: Add type declarations to Html::decodeEntities() and Html::escape() @borisson_ brings up concerns of this being a BC break. I kinda agree. Today tests fail because we treat unexpected deprecation as errors but sites continue to function. This patch flips that and its always a fatal error breaking sites.

The problem with these two issues is Drupal needs to communicate changes while continuing the intent of a thin wrapper around the PHP methods. That's kinda unique because of how this class is designed but also pretty similar to a lot of other PHP 8.1 and 8.2 issues floating around the queue. <rant></rant>

As I noted in #11, it feels very weird to add a deprecation that effectively wraps a PHP deprecation. Putting the gut reaction aside, the fact is it separates PHP's deprecation timeline from our deprecation timeline. That means it is probably the right decision for the reasons @borisson_ mentions.

That said, depending on how fast this moves and maintainer buy in, that path might make the interface change _faster_ than PHP's because these changes could land in 9.5 and and interface change 10 meeting the "10 is 9 - deprecation" promise.

Here's a 9.5 deprecation patch to see where it goes.

Fixed Drupal CS issue of patch #57.

This issue is targetting 10.0.x but the latest patch says "deprecated in drupal:9.5.0 and is removed from drupal:10.0.0".

Liam, the new trigger_error talks of the 'passing of null' being deprecated, not the whole routine.

I must admit, though, I find the wording ambiguous... what exactly does 'is removed from' mean? I think it would be clearer if it became 'will cause a php Error' or similar.

OK, I see. It is not that something gets removed, but that the required PHP upgrade causes the error to happen.

Patch #58 applied to 9.4.5 and seems to be working. I had multiple errors on the extend page.
They are gone now.

It is too late to add this to 9.5.x or 10.0.x, this will have to go into 10.1.x for removal in 11.0.x now.

Addressed comment #63.

Perhaps "is removed from" should instead be "is prohibited starting in".

Addressed comment #67.

I realise that this issue is now focussed on 10.1.x but the patch at #68 applied to 9.4.8 and has stopped the deprecation errors flagging up

I have observed a very similar issue when updating a Drupal 9.4.8 website to use PHP 8.1 (moving from PHP 7.4). After updating to PHP 8.1, if I visit the "Available Updates" page in the Drupal admin (/admin/reports/updates), the dblog becomes littered with php debug messages. The stack trace is:

Deprecated function: htmlspecialchars(): Passing null to parameter #1 ($string) of type string is deprecated in Drupal\Component\Utility\Html::escape() (line 424 of /var/www/html/web/core/lib/Drupal/Component/Utility/Html.php)
#0 /var/www/html/web/core/includes/bootstrap.inc(347): _drupal_error_handler_real(8192, 'htmlspecialchar...', '/var/www/html/w...', 424)
#1 [internal function]: _drupal_error_handler(8192, 'htmlspecialchar...', '/var/www/html/w...', 424)
#2 /var/www/html/web/core/lib/Drupal/Component/Utility/Html.php(424): htmlspecialchars(NULL, 11, 'UTF-8')
#3 /var/www/html/web/core/lib/Drupal/Component/Render/FormattableMarkup.php(262): Drupal\Component\Utility\Html::escape(NULL)
#4 /var/www/html/web/core/lib/Drupal/Component/Render/FormattableMarkup.php(208): Drupal\Component\Render\FormattableMarkup::placeholderEscape(NULL)
#5 /var/www/html/web/core/lib/Drupal/Core/StringTranslation/TranslatableMarkup.php(195): Drupal\Component\Render\FormattableMarkup::placeholderFormat('Includes: @rend...', Array)
#6 /var/www/html/web/core/lib/Drupal/Component/Utility/ToStringTrait.php(15): Drupal\Core\StringTranslation\TranslatableMarkup->render()
#7 /var/www/html/web/sites/default/files/php/twig/635aca39a7fc0_update-project-status.htm_ZtQ542K6kzk5EmIQb92x0Omz9/SJiARl9ctDhvdjO9taMqrc2IFW4PfWdMl_DUT4DvKNo.php(189): Drupal\Core\StringTranslation\TranslatableMarkup->__toString()
#8 /var/www/html/vendor/twig/twig/src/Template.php(405): __TwigTemplate_1fc8957a5664ce1602106953f9dbd3e9->doDisplay(Array, Array)
#9 /var/www/html/vendor/twig/twig/src/Template.php(378): Twig\Template->displayWithErrorHandling(Array, Array)
#10 /var/www/html/vendor/twig/twig/src/Template.php(390): Twig\Template->display(Array)
#11 /var/www/html/web/core/themes/engines/twig/twig.engine(55): Twig\Template->render(Array)
#12 /var/www/html/web/core/lib/Drupal/Core/Theme/ThemeManager.php(384): twig_render_template('core/modules/up...', Array)
#13 /var/www/html/web/core/lib/Drupal/Core/Render/Renderer.php(422): Drupal\Core\Theme\ThemeManager->render('update_project_...', Array)
#14 /var/www/html/web/core/lib/Drupal/Core/Render/Renderer.php(201): Drupal\Core\Render\Renderer->doRender(Array, false)
#15 /var/www/html/web/core/lib/Drupal/Core/Template/TwigExtension.php(479): Drupal\Core\Render\Renderer->render(Array)
#16 /var/www/html/web/sites/default/files/php/twig/635aca39a7fc0_table.html.twig_dgp2fXo7tD-zl5dhJdnz9-5ei/eevEC02gZE_QFtAqZ3SLFFMgz8poR9oBH8kJUPA8ub8.php(196): Drupal\Core\Template\TwigExtension->escapeFilter(Object(Drupal\Core\Template\TwigEnvironment), Array, 'html', NULL, true)
#17 /var/www/html/vendor/twig/twig/src/Template.php(405): __TwigTemplate_4e1d4e4d704894ba45c4da187961f3fd->doDisplay(Array, Array)
#18 /var/www/html/vendor/twig/twig/src/Template.php(378): Twig\Template->displayWithErrorHandling(Array, Array)
#19 /var/www/html/vendor/twig/twig/src/Template.php(390): Twig\Template->display(Array)
#20 /var/www/html/web/core/themes/engines/twig/twig.engine(55): Twig\Template->render(Array)
#21 /var/www/html/web/core/lib/Drupal/Core/Theme/ThemeManager.php(384): twig_render_template('themes/composer...', Array)
#22 /var/www/html/web/core/lib/Drupal/Core/Render/Renderer.php(422): Drupal\Core\Theme\ThemeManager->render('table', Array)
#23 /var/www/html/web/core/lib/Drupal/Core/Render/Renderer.php(201): Drupal\Core\Render\Renderer->doRender(Array, false)
#24 /var/www/html/web/core/lib/Drupal/Core/Template/TwigExtension.php(479): Drupal\Core\Render\Renderer->render(Array)
#25 /var/www/html/web/sites/default/files/php/twig/635aca39a7fc0_update-report.html.twig_ijVG41Nr_4oX-5Vx7u1IvTf3n/QO9Cc-HCTkgwuSvc-w8iWa67otJJINV9EhfJazJukL8.php(54): Drupal\Core\Template\TwigExtension->escapeFilter(Object(Drupal\Core\Template\TwigEnvironment), Array, 'html', NULL, true)
#26 /var/www/html/vendor/twig/twig/src/Template.php(405): __TwigTemplate_b34bddf097fbb58e712deae581381c3b->doDisplay(Array, Array)
#27 /var/www/html/vendor/twig/twig/src/Template.php(378): Twig\Template->displayWithErrorHandling(Array, Array)
#28 /var/www/html/vendor/twig/twig/src/Template.php(390): Twig\Template->display(Array)
#29 /var/www/html/web/core/themes/engines/twig/twig.engine(55): Twig\Template->render(Array)
#30 /var/www/html/web/core/lib/Drupal/Core/Theme/ThemeManager.php(384): twig_render_template('core/modules/up...', Array)
#31 /var/www/html/web/core/lib/Drupal/Core/Render/Renderer.php(422): Drupal\Core\Theme\ThemeManager->render('update_report', Array)
#32 /var/www/html/web/core/lib/Drupal/Core/Render/Renderer.php(201): Drupal\Core\Render\Renderer->doRender(Array, false)
#33 /var/www/html/web/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php(241): Drupal\Core\Render\Renderer->render(Array, false)
#34 /var/www/html/web/core/lib/Drupal/Core/Render/Renderer.php(564): Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}()
#35 /var/www/html/web/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php(242): Drupal\Core\Render\Renderer->executeInRenderContext(Object(Drupal\Core\Render\RenderContext), Object(Closure))
#36 /var/www/html/web/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php(132): Drupal\Core\Render\MainContent\HtmlRenderer->prepare(Array, Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\Core\Routing\CurrentRouteMatch))
#37 /var/www/html/web/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php(90): Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse(Array, Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\Core\Routing\CurrentRouteMatch))
#38 [internal function]: Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object(Symfony\Component\HttpKernel\Event\ViewEvent), 'kernel.view', Object(Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher))
#39 /var/www/html/web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php(142): call_user_func(Array, Object(Symfony\Component\HttpKernel\Event\ViewEvent), 'kernel.view', Object(Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher))
#40 /var/www/html/vendor/symfony/http-kernel/HttpKernel.php(164): Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object(Symfony\Component\HttpKernel\Event\ViewEvent), 'kernel.view')
#41 /var/www/html/vendor/symfony/http-kernel/HttpKernel.php(81): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request), 1)
#42 /var/www/html/web/core/lib/Drupal/Core/StackMiddleware/Session.php(58): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#43 /var/www/html/web/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(48): Drupal\Core\StackMiddleware\Session->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#44 /var/www/html/web/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(48): Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#45 /var/www/html/web/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(51): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#46 /var/www/html/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#47 /var/www/html/web/core/lib/Drupal/Core/DrupalKernel.php(709): Stack\StackedHttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#48 /var/www/html/web/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request))
#49 {main}

Applying the patch from #68 to Drupal core seems to stop these messages from accumulating in the dblog.

I am loath to suggest changing this, as this issue has gone on far too long.

However, "prohibited" is not in my opinion the best option here. I would prefer "trigger a PHP error from", as in:

...deprecated in drupal:10.1.0 and is prohibited starting in drupal:11.0.0. Pass a string or ...

becomes:

...deprecated in Drupal:10.1.0 and will trigger a PHP error, starting from Drupal:11.0.0. Pass a string or ....

This updated version of the patch is attached. I've also changed 'drupal' to 'Drupal' -- proper names have capital letters.

We also need to address the problem of Drupal 9 sites running with php8.1, which will be the normal state soon if not now, and do so quickly. Dropping lots of stack traces into the system log is not a good look!

At least at present, the existing patch 3255637-68.patch nearly applies cleanly to HEAD of 9.5.x (f3a8b0dadc4). I have uploaded a version that does apply cleanly, using extension "nopatch" because this issue is versioned for D10!

Marking Critical for visibility -- on D9 mostly -- happy to downgrade again :-)

+++ b/core/lib/Drupal/Component/Utility/Html.php
@@ -420,8 +423,11 @@ public static function decodeEntities($text) {
+      trigger_error('Passing null to Html::escape() is deprecated in Drupal:10.1.0 and will trigger a PHP erorr from Drupal:11.0.0. Pass a string instead. See https://www.drupal.org/project/drupal/issues/3255637.', E_USER_DEPRECATED);

This line is causing a cspell failure (see https://www.drupal.org/pift-ci-job/2509141):

/var/www/html/core/lib/Drupal/Component/Utility/Html.php:428:107 - Unknown word (erorr)

CSpell: failed

I have fixed CS error. Please review it
And address #73

Patch #74 applies to 9.4.8 with PHP8.1 - Deprecation errors do not appear

+1 RTBC

Tested on Drupal 10.1.x and php 8.1

- The issue description is clear
- The patch applies cleanly and fixes the issue
- Patch addresses issues mentioned above
- No issues identified with the code

Marking this as RTBC

+++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
@@ -247,13 +247,21 @@ protected static function placeholderFormat($string, array $args) {
+        'Passing null to ' . __METHOD__ . ' is deprecated in Drupal:10.1.0 and will trigger a PHP error from Drupal:11.0.0. Pass a string or ' . MarkupInterface::class . ' instead. See https://www.drupal.org/project/drupal/issues/3255637.',

Deprecations use lowercase "drupal", this should be "drupal:10.1.0" and "drupal:11.0.0". Also, these deprecations need tests.

I think we should consider backporting this to 9.5.x (for removal in Drupal 11) even though it's technically too late, since the end result is it reduces the errors people see (on PHP 8.1 and higher) rather than just adding a new deprecation. That way modules can deal with the deprecation without bothering site owners. Discussed briefly with @longwave and slack and he agreed, so marking needs work for that.

Also some review issues:

1. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -205,7 +205,7 @@ protected static function placeholderFormat($string, array $args) {
              // \Drupal\Component\Render\MarkupInterface, so this placeholder type
              // must not be used within HTML attributes, JavaScript, or CSS.
   -          $args[$key] = static::placeholderEscape($value);
   +          $args[$key] = static::placeholderEscape($value ?? '');
              break;
    
   

   This is escaping an empty string for no reason, I think it should be = $value ? static::placeholderEscape($value) : ''; or similar.

2. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -247,13 +247,21 @@ protected static function placeholderFormat($string, array $args) {
   +  protected static function placeholderEscape($value): string {
   +    if (is_null($value)) {
   +      trigger_error(
   +        'Passing null to ' . __METHOD__ . ' is deprecated in Drupal:10.1.0 and will trigger a PHP error from Drupal:11.0.0. Pass a string or ' . MarkupInterface::class . ' instead. See https://www.drupal.org/project/drupal/issues/3255637.',
   +        E_USER_DEPRECATED
   +      );
   +      return '';
   

   Message looks fine, let's just update to 9.5.x. Could also use a deprecation test.

3. +++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
   @@ -202,7 +202,7 @@ public function generate($text, Url $url) {
        if (!($variables['text'] instanceof MarkupInterface)) {
   -      $variables['text'] = Html::escape($variables['text']);
   +      $variables['text'] = Html::escape($variables['text'] ?? '');
        }
   

   Same thing here - let's not escape an empty string.

Updated patch from #74 with suggestions from #78 and #79.

I suggest isset() instead of !empty($value) otherwise it will not give the correct behavior if the value is zero.

Agree, isset() or is_null() should be used. empty(x) is not sufficient.

The new update has changed Drupal->drupal once but not twice as needed (this is why I don't like lines >100 chars long).

What's the logic of lower-case drupal here anyway? Drupal is a proper name => has an initial capital.

Addressed comments from #81 and #82 as well as a missing use statement and some coding standards.

Test fixes.

Fixed interdiff.

1. +++ b/core/lib/Drupal/Component/Utility/Html.php
   @@ -382,7 +382,10 @@ public static function escapeCdataElement(\DOMNode $node, $comment_start = '//',
   +      trigger_error('Passing NULL to ' . __METHOD__ . ' is deprecated in drupal:9.5.0 and will trigger a PHP error from drupal:11.0.0. Pass a string instead. See https://www.drupal.org/project/drupal/issues/3255637', E_USER_DEPRECATED);
   

   Can we return '' early here?

2. +++ b/core/lib/Drupal/Component/Utility/Html.php
   @@ -420,8 +423,11 @@ public static function decodeEntities($text) {
   +      trigger_error('Passing NULL to ' . __METHOD__ . ' is deprecated in drupal:9.5.0 and will trigger a PHP error from drupal:11.0.0. Pass a string instead. See https://www.drupal.org/project/drupal/issues/3255637', E_USER_DEPRECATED);
   

   Can we return '' early here instead of the $text check below?

Kim, re:

(1) in escapeCdataElement, currently $text isn't being modified at all. It would make sense it would be, especially as there is a trigger_error, but I don't actually know that NULL input is banned in html_entity_decode().

(2) in #88 that seems sensible, indeed.

Addressed suggestions from #88 and fixed the failing Html utility tests. Not really sure if there is a way to test the deprecation triggered within FormattableMarkup::placeholderEscape() since the changes to FormattableMarkup::placeholderFormat() now prevent NULL values from being passed to FormattableMarkup::placeholderEscape(). Maybe the deprecation trigger warning trigger_error() should be moved to FormattableMarkup::placeholderFormat() or the contsructor method instead?

The link in the deprecation message needs to point to a change record. I have created a new draft one here https://www.drupal.org/node/3318826 that you can link to.

+++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
@@ -205,7 +205,7 @@ protected static function placeholderFormat($string, array $args) {
+          $args[$key] = isset($value) ? static::placeholderEscape($value) : '';

I think this change isn't needed now as placeholderEscape always returns a string now.

You can remove the test for it too.

Incorporated suggestions from #91.

I think this change isn't needed now as placeholderEscape always returns a string now.

Should we remove the isset() checks added to calls to Html::escape() and Html::decodeEntities() as well now that those also always return strings?

@joegraduate, we still need the isset() calls in the code path leading to html::escape(), because the problems exist on *input* to escape et al, not the function's return value.

Doing the right thing on input is something that will become critical in D11 when the deprecation code is removed and (possibly) the function arg type is changed to 'string $text' rather than just '$text' or '?string $text'. Even now, when the deprecation code is hiding the problem, you get php errors in logs.

Thanks @rivimey. Does that mean we need to revert the change suggested in #91?

In #91 we have the line:

$args[$key] = isset($value) ? static::placeholderEscape($value) : '';

What this line says is:

If the variable called 'value' is set (that is, 'value' exists and is not null), then

  - call placeholderEscape(), passing the variable 'value' contents as the first argument
  - save the function call result as the expression result.

Otherwise (if value does not exist or is null) then

  - save the empty string constant "" as the expression result.

Write the expression result (from the call or the constant) to the array 'args' at the array slot given by the contents of variable 'key'.

In doing this we avoid passing the value NULL into placeholderEscape(), as required by the updated code.

Put simply, the change highlighted in #91 is good and it is needed.

Tagging for bugsmash

I reviewed patch and it looks good. Two things:

1. I can't see the issue against Drupal 11 where the wrappers will be removed. Has this been created?
2. the CR "is deprecated in Drupal 9.5.0 and will throw an error in Drupal 10.0.0", the deprecation message "is deprecated in drupal:9.5.0 and will trigger a PHP error from drupal:11.0.0". Should they align to Drupal 11?

gambry, the message was changed to 'trigger a php error' because in D11 the intention is to remove the trigegr_error()s and tighten up the type definitions such that php will complain about type failure. Consequently there won't be any 'throw()' or similar; it will just be 'plain' code.

Yes, definitely the CR should match... I didn't know it was a thing to create specific issue for removing wrappers. Should such an issue be created *before* the code to be removed is even in Drupal?

the message was changed to 'trigger a php error' because [...]

Hey @rivimey. Yep, I was referring to the drupal versions mismatch.

I didn't know it was a thing to create specific issue for removing wrappers. Should such an issue be created *before* the code to be removed is even in Drupal?

Well, I thought that was the practice. But apparently I dreamed about it since is not on Drupal deprecation policy.

I've updated the CR to reflect the deprecation message: "[...] This behaviour is deprecated in Drupal 9.5.0 and will trigger a PHP error from Drupal 11.0.0."

Thanks everyone.

This is resulting in a phpstan failure on 10.1.x:

Running PHPStan on changed files.
 ------ --------------------------------------------------------------- 
  Line   lib/Drupal/Component/Render/FormattableMarkup.php              
 ------ --------------------------------------------------------------- 
  223    Variable $value in isset() always exists and is not nullable.  
 ------ --------------------------------------------------------------- 


                                                                                
 [ERROR] Found 1 error                                                          
                                                                                


PHPStan: failed

Probably needs to be an explicit is_null() if the check is needed.

So PHPStan is right. The lines in question are:

          $value = UrlHelper::stripDangerousProtocols($value);
          $args[$key] = isset($value) ? Html::escape($value) : '';

UrlHelper::stripDangerousProtocols() is documented as taking and returning a string; it also can't handle NULL:

>>> \Drupal\Component\Utility\UrlHelper::stripDangerousProtocols(NULL);
PHP Deprecated:  strpos(): Passing null to parameter #1 ($haystack) of type string is deprecated in /var/www/html/drupal/core/lib/Drupal/Component/Utility/UrlHelper.php on line 358

I wonder if we should just always cast a NULL $value to the empty string for all cases in ::placeholderFormat()? It's the only thing that makes any real sense to me; as we are dealing with array values, we can't enforce types in the method signature.

The latest patch also doesn't have tests for the changes to ::placeholderFormat() or the deprecation in ::placeholderEscape(). Ideally I think we want to test NULL values for all the possible placeholder types?

Implemented #101; NULL values in the arguments are always cast to empty string, and added tests for each placeholder type. I think ::placeholderEscape() no longer needs a deprecation; it's protected so can only be called by the same class or subclasses, and we've fixed known callers here.

For core where this happened when were updating to support PHP 8.1 and 8.2 I pushed back against this type of fix because it always traced back to a message where actually doing an empty string replacement made no sense. If we hide errors here we're not listening to the system telling us our assumptions are wrong.

+++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
@@ -196,6 +196,11 @@ public function jsonSerialize() {
+      // Transform NULL values to the empty string.
+      if (is_null($value)) {
+        $value = '';
+      }

At the very least, I think this should trigger a deprecation too.

Yes, when I had one of these in contrib modules it was usually a hidden bug, sometimes only in tests due to missing config or so. Tracking them down can however be very tricky, due to our delayed rendering and generation of translated strings.

So, +1 on a deprecation and later (D11?) possibly an exception, but both should contain the string with the placeholders and the specific placeholder causing the problem IMHO, that would make it a lot easier to find where they are coming from.

#104 is a great idea! Making it easier to track this down could/will save people a lot of time.

NW for #103/104

Here's a patch that replaces longwave's changes to FormattableMarkup.php following the suggestions made after that, but retains the additional test in FormattableMarkupTest.php.

I have also taken the chance to improve the function docs for placeholderFormat().

Oops, wrong patch root.

Fix style complaints

The test failure is from the code @longwave added and I'm not sure what the correct resolution is.

I don't think the test case for ['<em class="placeholder"></em>', '%empty', ['%empty' => NULL]], is required. I've removed it to fix the issue

Most of the docs changes appear to be out of scope - let's try and keep this patch focused on solving the issue at hand, docs improvements can be done separately.

I added the test case as we need to know what should happen in the case NULL is passed and a % placeholder is used. Unsure if it should return an empty string entirely, or an empty <em> tag?

I don't think it ever makes semantic sense to have an empty em element.

Emphasized emptiness sounds like the sound of one hand clapping.

That is what passing NULL or the empty string currently return:

>>> (string) new \Drupal\Component\Render\FormattableMarkup('%empty', ['%empty' => NULL]);
=> "<em class="placeholder"></em>"

>>> (string) new \Drupal\Component\Render\FormattableMarkup('%empty', ['%empty' => '']);
=> "<em class="placeholder"></em>"

This would be a behaviour change - and do we change the empty string at the same time for consistency?

That change should probably be a separate issue so that this one can focus on the type declarations.

Lets focus on making the code behave well without changing its meaning. If people feel there is a bug to be squashed with e.g. empty tags then by all means create an issue to fix it. This issue has already feature-creeped itself into the next version of Drupal!

This hasn't addressed htmlspecialchars_decode(), which has a similar problem since PHP 8.1, mainly because Html does not have a wrapper for htmlspecialchars_decode(). Can we add a wrapper and solve it in whatever manner is decided for other methods.

There are several issues that would have been solved in a single place if we had this wrapper
* #3301782: PHP8 Deprecated function : htmlspecialchars(): Passing null to parameter #1 ($string) of type string is deprecated
* #3298778: PHP 8.1 Deprecated function: htmlspecialchars(): Passing null to parameter #1 ($string) of type string is deprecated
* #3324460: Deprecated function: htmlspecialchars() with PHP 8.1 when viewing schedules

Patch attached fixes #113 to not change the output of %placeholder when the value is NULL. It tests the deprecations for all placeholders. And removes unnecessary change. For example, it removes the stricter typehinting on Html::escape() - it's out-of-scope and also would be a behaviour change - see https://3v4l.org/2fkmR

The patch attached works on 9.5.x and 10.1.x :)

Removing more unnecessary change.

+++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
@@ -196,6 +196,14 @@ public function jsonSerialize() {
+        @trigger_error(sprintf('Deprecated NULL placeholder value for key (%s) in: "%s". This will throw a PHP error in drupal:11.0', (string) $key, (string) $string), E_USER_DEPRECATED);

Probably should say "drupal:11.0.0" to match other version references, and also end with a full stop.

Otherwise this looks good and would be great to get into 9.5.0.

I've rewritten the summary to detail the latest approach as based on #104

@longwave good point. Fixed and pointed to CR.

Thanks, even better. Everything looks good, this should help track down the root cause of multiple different issues.

Slight improvement on one of the comments.

This looks good now.

I think there's a question whether we want to throw an exception or move to an unsilenced E_USER_WARNING or similar for 11.x, but we can decide that when we get there.

This is technically a new deprecation in 9.5.x, but it's replacing an unsilenced one with a silenced one, unless you're still on PHP 7.4, but it's still silenced on PHP 7.4 too.

Committed/cherry-picked/pushed to 10.1.x, 10.0.x, and 9.5.x, thanks!

@alexpott, @longwave @catch Given that some of the changes were removed (a good thing), have issues been raised to remind us that they were considered a problem? Even a bullet-point summary here of what was dropped would be helpful.

The owner of merge request !1657 will need to close it.

An empty string field value is NULL, if that makes its way to Html::escape , that will trigger this notice. For example t('@foo', ['@foo' => $node->foo->value]) goes to TranslatableMarkup::placeholderFormat() and then FormattableMarkup::placeholderEscape and then straight to Html::escape($value). NestedArray::getValue() will also return a NULL on key not found and that's not necessarily a bug either. parse_url does "If the requested component doesn't exist within the given URL, null will be returned". And so on.

While it's possible to patch the code chain lined up above, there are any number of ways where a completely valid NULL can make its way to Html::escape(). It is a complete waste of time to hunt down every single one. This should be rolled back and a silent string cast added. (It was a bad idea for PHP as well, they are hell bent lately on adding a lot of completely needless busywork to upgrade, we do not need to copy theirs, rather we should make the life of developers easier.)

In general, this issue attacked the problem from the wrong end. If we want to steer Drupal into a strict typed land then we need to take a look at our data producers first and only then change the consumers.

A custom entity type without a label will trigger the error mentioned by @ghost-of-drupal-past when using a DraggableListBuilder and when saving the entity using EntityForm. I'll open a separate issue, but the solution might be a rollback of this change.