Html::decodeEntities() and Html::escape() can only take strings as their parameters because they pass the value to PHP functions that can only take strings. Sometimes, NULL is passed, leading to errors in PHP 8.1. By adding type declarations, the source of these errors can be identified and fixed.
In FillPDF tests on PHP 8.1, I am seeing errors like this:
1) Drupal\Tests\fillpdf\Functional\HandlePdfControllerTest::testSaveAction
Exception: Deprecated function: htmlspecialchars(): Passing null to parameter #1 ($string) of type string is deprecated
Drupal\Component\Utility\Html::escape()() (Line: 424)
The FillPDF code that triggers this is an ordinary call to drupalPostForm(). Parameter $form_html_id is NULL, but this value does not end up getting passed to Html::escape().
It does not look to me like there is any way I could fix this within FillPDF. So, I think this is a core bug. Similar errors were part of #3241280: Fix PHP 8.1 deprecations caused by views code.
| Comment | File | Size | Author |
|---|---|---|---|
| #25 | 3271517-9.4.x-htmlspecialchars.patch | 549 bytes | emil stoianov |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
compare
Comments
Comment #3
liam morlandComment #7
bserem commentedComment #9
bserem commentedAdded a second MR against core 9.4.x
Comment #10
smustgrave commentedMoving to 10.1.x as this seems like a PHP8 issue and D9 still supports php7.4 not sure if this fix will break those versions.
Also could use a test case.
Comment #11
liam morlandI have rebased my fix.
Adding
?? ''will clear the error but not solve the underlying problem. Adding the type declarations means the error reporting will say what other code is sending non-strings into these functions.What kind of test could be added? This gets tested by the existing tests that test these functions.
Comment #12
smustgrave commentedIf this is a bug then the test cases should of caught it. So could use one that would.
Comment #13
liam morlandWhat would the test look like? Will it allow a test that deliberately breaks a type declaration and checks that an error is raised?
Comment #14
smustgrave commentedSound right. If you pass in a non string verify the return matches the error
Comment #15
liam morlandPass NULL to the function and the test has to pass if PHP throws as error. How would one write a test for that?
Comment #17
liam morland@bserem If this is working for you, perhaps you could mark it RTCB.
Comment #18
borisson_You can assert for a
TypeErrorto appear, but I'm not sure if we should actually go trough with this issue. See https://www.drupal.org/project/drupal/issues/3050720#comment-13092605 for example, adding type hints are bc breaks so we will have first decided on when and in what manner we should add them. I'd suggest we first get that issue moving again and then create tickets to do it in bigger blocks? Doing it method-per-method or even with just one class feels like this is too small. I'd suggest we do this by adding a rector or phpcs rule instead and automate as much as possible?Comment #19
liam morlandThis one is not a BC break. As it is, if you pass NULL to decodeEntities() or escape(), you will get an error, but the error will not be very useful because it will just say that decodeEntities() or escape() called html_entity_decode() or htmlspecialchars() with NULL. By adding the type declaration, the error will be reported at the place were decodeEntities() or escape() was called. This allows those places to be fixed.
A comprehensive patch to add type declarations everywhere would be a very big undertaking and will take a long time. In the meantime, there will be errors about NULL being passed into a string and it will be hard to figure out where they are coming from. Fixing this issue now will make those errors easy to fix and demonstrate the value of type declarations.
Comment #20
neclimdulSeems like y'all are just rehashing the discussions on #3255637: Deprecate NULL values in Html::escape(), ::decodeEntities(), and FormattableMarkup::placeholderFormat() to make it easier to upgrade to PHP 8. This just has decodeEntities and misses placeholder escape. Feels like we should close this and continue over there since its further along.
Comment #21
smustgrave commentedAgree with #20
Comment #22
liam morlandOne important difference is that the other one is more complicated and involves union types. This one is very simple. It just exposes the type requirements that exist anyway. It does not cause any backwards-compatibility problems. Committing this fix would be very helpful while not impeding the work on the other issue.
Comment #23
neclimdulIt only has union types because this issue didn't address that the escape change affects placeholderEscape. So it is not complete and as mentioned behind the other patch. I discussed why even this patch _actually_ breaks BC in the other issue. Summary, today tests fail. With this patch sites fatal.
Comment #24
liam morlandTests pass for the changes in merge request 2130. This merge request is targeting Drupal 10, so PHP 8.1 is required. This patch is only surfacing type constraints that already exist. This does not cause any new error messages to be generated; it only causes the error messages to be more useful.
Even without this patch, passing a non-string to decodeEntities() or escape() would cause an error. But the error would be reported inside whichever of those functions was called, making it hard to track down where the problem comes from. With the patch, the error would be reported at the caller of decodeEntities() or escape().
Comment #25
emil stoianov commentedProviding the same fix as in MR !2547 (for 9.4.x) but as a patch
Comment #26
alvarito75 commentedI confirmed this works, see below my setup:
Comment #27
longwaveRe #24 in PHP 8.1, passing a NULL to those methods currently causes a deprecation notice, but not an error - code continues to execute:
But once the MR is applied, this becomes a fatal error:
MR!2130 passes because core never passes NULL in normal operation - but almost certainly, contrib and custom code does. We should add a test to cover this case!
So: as #18 says, this is a BC break. The better, albeit slower, way to deal with it is in #3255637: Deprecate NULL values in Html::escape(), ::decodeEntities(), and FormattableMarkup::placeholderFormat() to make it easier to upgrade to PHP 8 and I think this should be closed as a duplicate of that.
Comment #28
liam morlandI agree. Sorry for overlooking that the MR changes the deprecation notice to a fatal error.
Duplicate of #3255637: Deprecate NULL values in Html::escape(), ::decodeEntities(), and FormattableMarkup::placeholderFormat() to make it easier to upgrade to PHP 8.