FormattableMarkup: NULL @ and % placeholder values should log a notice and use empty string

This adds the type-hint, and checks if the input is NULL.

Will need test coverage for this one. Also may want to investigate #3 if how much they overlap.

Can confirm the patch in #4 solved my issue for the same error I had when creating new media content when I upgraded to Drupal 11.2.9. Thank you @robloach

Created an Issue fork and merge request including the code from #4 and added test coverage for placeholder set to NULL.

I found that The existing patch in #4 only partially fixes this issue. It handles NULL values in `placeholderEscape()` for `@` and `%` placeholders, but misses the `:` placeholder case (used for URLs).

Setting to needs review.

The NULL handling in FormattableMarkup revealed some edge cases where code was passing NULL values to placeholders. I've fixed these as part of this issue:

1. **MediaLibraryWidget.php** (lines 430, 437): Now checks that `$media_item->label()` is not NULL before using it in placeholders. Falls back to generic "Remove media" / "Removing media." messages when label is NULL.

2. **BlockEntitySettingTrayForm.php** (lines 40, 84): Now checks that `admin_label` exists before using it in placeholders. Falls back to "Configure block" / "Save block" when admin_label is NULL or empty.

These changes ensure that when FormattableMarkup receives NULL placeholder values, the calling code provides appropriate fallback messages rather than producing incomplete output like "Configure " or "Removing .".

The updated test coverage in FormattableMarkupTest::testNullPlaceholder() verifies that NULL values are handled correctly for all placeholder types (@, %, :).

Left some comments on the MR

Thanks!

Okay, I've taken note of the comments and updated the code accordingly. Setting back to Needs Review. Thanks @smustgrave

I cleaned up my code changes and removed what was out of scope that I originally had to make the tests pass. I wrote better testing to test the NULL placeholder values and test NULL values in @ and % placeholder to trigger deprecation.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Okay, fixed php unit tests errors and set the correct branch to 11.x for MR. Setting to Needs Review.

Updating target version to 11.x since this adds a deprecation (not removes code).
This will be included in Drupal 11.4.0 and the deprecated code path will be removed in a follow-up issue for Drupal 12.0.0.

issues are actually landing in main first now.

Okay, I couldn't get the tests to run on the pipeline on the Merge Request so I switched it to 11.x and it worked for me. Not sure if that was the right move?

They should be in sync. But yes will have to be mergable to main. That’s what everything has been getting merged to for a few weeks

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Hello all, I have changed the targeted branch to main.

I haven't done a thorough review of the MR, but I skimmed it and noticed the use of #[Group('legacy')] in the new tests. #[IgnoreDeprecations] should be used instead. See #3549970: Use #[IgnoreDeprecations] instead of #[Group('legacy')].

There's no need for apology. I was reviewing the code, not actively working on it. Thank you for your work to improve Drupal.

Please see my previous comment where I set the status to Needs Work. We no longer use #[Group('legacy')] for deprecation tests. We use #[IgnoreDeprecations] instead.

Also, the parameter type hints should be added to the function declarations too, e.g. public function testNullPlaceholderDeprecated(string $string, array $arguments, string $expected): void {.

Hello @dcam, Good catch, I have updated #[Group('legacy')] with #[IgnoreDeprecations]. Also, the parameter type hints are already included for the function testNullPlaceholderDeprecated.

Please review it.

I added the type hinting on the function params. Thanks @samitk @dcam for your contrib. I'll set this to review.

We had a similar error with a view that was ok in frontend but could not edit in admin after upgrading from drupal 10 to 11.

We have applied this merge request plain diff as a patch on Drupal 11.3.3 and it solved our problem.

We were then able to edit the view and see a warning
User deprecated function: Passing NULL as a placeholder value is deprecated in drupal:11.4.0 and is removed from drupal:12.0.0

Turned out the problem was a NULL "item per page" on the mini pager. When using "0" the warning disapear and view is working fine.
I tried setting items per page to NULL and I could save the form and see the warning from this issue again, that's another issue but maybe views should not allow to save the form if "items per page" is NULL?

EDIT: Yes I have found the issue with the "item per page" problem https://www.drupal.org/project/drupal/issues/3564838

Applied https://www.drupal.org/project/drupal/issues/3554974#comment-16338092 to my recent D11 upgrade to 11.3.5. Works fine on mine. Thanks

Resolved the open threads I opened. To me this seems like it could be ready @dcam what do you think?

I started a code review on the MR, but eventually realized that this issue has deeper problems.

• The issue title doesn't do much to explain the work being done. A reviewer will arrive on this page expecting changes related to loggers, then finds a fundamental core system related to security being modified.
• The proposed resolution section of the issue summary is out-of-date. Tagging for issue summary updates. Honestly I feel like the whole summary needs some love. Currently it's confusing in the same way that the issue title is. Start out by explaining what the problem is, not by giving an example of what causes it to happen.
• I went around and around on trying to decide what to do with these changes until I finally realized that this fix is in the wrong place. It attempts to correct a problem by changing a second-level protected utility function. Why? Let's not make it the responsibility of placeholderEscape() to sanitize bad input. And per the backward compatibility policy we should be able to add type hints to it at any time without deprecation to enforce good input because it's internal.

  placeholderFormat() is the function that parses out the arguments and calls placeholderEscape(). That is where we should be checking that the arguments meet expectations, at the top of its foreach loop. Assuming that we decide FormattableMarkup should be responsible for sanitization*, then that's where you would do it. At that point having a deprecation no longer makes sense because we aren't intending to remove that sanitization in the future and we aren't changing any type hints.

• The change record needs to be updated in any case, but if my idea in the previous bullet is implemented then it especially does. It should focus on the change in behavior - the switch from NULL arguments triggering errors to being replaced with empty strings. Then mention that calling code needs to ensure proper input.

* I think it should throw an InvalidArgumentException if an improper value is encountered and force calling code to fix their inputs, but that's just me.

Discussed with @smustgrave that if we move forward with having placeholderFormat() sanitize values, then it should log something. I'm not sure what log level without looking at the PSR documentation, maybe "notice" or "debug". Because otherwise there's no information at all that this is happening. But if we start logging things, then maybe we can get fixes pushed to code that calls this with improper input.

I made changes based on the comments from @dcam.

• Moved the check to placeholderFormat() away from the internal placeholderEscape() call.
• Added E_USER_NOTICE as a log level on FormattableMarkup that is in Drupal\Component.
• Removed the deprecation and in favor of notice to alert developer to fix the issue in their code.
• Update Title and Proposed solution to relevant changes
• Updated change record description based on proposed solution

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Fixed phpcs

setting back to needs review

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Fixeed phpstan errors.

Instead of triggering a notice, could we assert(), which will cause an error for developers but then we can fall back to empty string after that in production where assertions should be disabled?

Seems like a plan @longwave.

Also MR should be rebased being 500+ back please

Okay, only took 2 rebases to get it right :). I updated the branch and switch to using assert rather than a notice. Tested using commands below.

Production:

php -d zend.assertions=-1 -r "
require 'autoload.php';
use Drupal\Component\Render\FormattableMarkup;
\$output = (string) new FormattableMarkup('Hello @name', ['@name' => NULL]);
echo var_export(\$output, true) . PHP_EOL;
"
Hello

Development:

php -d zend.assertions=1 -r "
require 'autoload.php';
use Drupal\Component\Render\FormattableMarkup;
echo (string) new FormattableMarkup('Hello @name', ['@name' => NULL]);
"
PHP Fatal error:  Uncaught AssertionError: Passing NULL as a placeholder value is not supported. Use an empty string instead. Placeholder: @name, String: "Hello @name". in 
#3 {main}