Problem/Motivation

ppl

In PHP 8.4, declaring functions/methods with parameters containing null as a default value, but without null as one of the types (either as a nullable syntax or as a Union type with null) is deprecated.

https://php.watch/versions/8.4/implicitly-marking-parameter-type-nullabl...

Steps to reproduce

Run Drupal in PHP 8.4.

Proposed resolution

Update all instances of such declarations to use Union types or nullable types.

This is a rather big set of changes. To replicate the results:

cd core
../vendor/bin/phpcbf --standard=SlevomatCodingStandard --sniffs=SlevomatCodingStandard.TypeHints.NullableTypeForNullDefaultValue lib modules themes profiles tests

or
php-cs-fixer fix . --rules nullable_type_declaration_for_default_null_value

Add SlevomatCodingStandard.TypeHints.NullableTypeForNullDefaultValue sniffer to prevent new issues

Remaining tasks

- decide on merge strategy (all at once after fixing manual set or split per module/system)
- patch/commit

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

Several fixes in Drupal core to fix the deprecation notices due on implicitly nullable function/method parameter declarations.

See:

CommentFileSizeAuthor
#8 3427999-d7-8.patch1.13 KBandypost

Issue fork drupal-3427999

Command icon Show commands

Start 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:

Comments

Ayesh created an issue. See original summary.

mfb’s picture

Ideally the phpdoc could stay in sync with the code?

e.g.

   * @param array $ids
   *   An array of entity IDs, or NULL to load all entities.
   *
   * @return static[]
   *   An array of entity objects indexed by their IDs.
   */
  public static function loadMultiple(array $ids = NULL);

becomes

   * @param ?array $ids
   *   An array of entity IDs, or NULL to load all entities.
   *
   * @return static[]
   *   An array of entity objects indexed by their IDs.
   */
  public static function loadMultiple(?array $ids = NULL);
mondrake’s picture

Priority: Normal » Major
Issue summary: View changes
Issue tags: +Major version only

Ideally should be solved before D11, since it has BC implications

poker10’s picture

andypost’s picture

First core run on 8.4 showed that we need to wait contrib fixes #3427903-3: [META] Make Drupal 10/11 compatible with PHP 8.4

andypost’s picture

Issue summary: View changes
andypost’s picture

StatusFileSize
new1.13 KB

D7 also needs fix

andypost’s picture

It affects in D10 hook_entity_field_access()

- function rest_test_entity_field_access($operation, FieldDefinitionInterface $field_definition, AccountInterface $account, FieldItemListInterface $items = NULL) {
+ function rest_test_entity_field_access($operation, FieldDefinitionInterface $field_definition, AccountInterface $account, ?FieldItemListInterface $items = NULL) {

andypost’s picture

Got a first with all deprecations filed (the failures are unrelated to the issue) https://git.drupalcode.org/issue/drupal-3427903/-/jobs/1459523

andypost’s picture

andypost’s picture

Filed remaining child issues, fill free to re-split/group

andypost’s picture

Issue summary: View changes

There's SlevomatCodingStandard.TypeHints.NullableTypeForNullDefaultValue sniffer to catch such issues

EDIT

A TOTAL OF 789 ERRORS AND 0 WARNINGS WERE FOUND IN 551 FILES

andypost’s picture

Discussed in slack with @catch and @smushgrave and decided to not split automated fixes

The issue is postponed follow-up #3444025: [8.4] Fix implicitly nullable type declarations in docblocks
That's because docblocks are not yet covered with sniffer #2572645: [Meta] Fix 'Drupal.Commenting.FunctionComment' coding standard

andypost’s picture

Issue summary: View changes
poker10’s picture

D7 also needs fix

Actually, as D7 still has to support PHP 5.6 and PHP 7.0, we would probably need to drop type declarations where needed and add type checks to the functions itself, see: https://php.watch/versions/8.4/implicitly-marking-parameter-type-nullabl...

Because nullable types were added only in PHP 7.1.

But thanks for the initial research. I will create a separate issues for D7 soon.

andypost’s picture

andypost changed the visibility of the branch 3427999-php-8.4-fix to hidden.

andypost’s picture

Status: Active » Needs review

Re-rolled and re-based https://git.drupalcode.org/project/drupal/-/merge_requests/8004 as reproducible set of commits

PS: Draft MR !7976 is just a attempt to run current state on PHP 8.4

bbrala’s picture

Seems you already mentioned the rector rule, awesome.

Think i might need to add that to drupal-rector, since it is a deprecation for 11, and its shouldn't break earlier code afaik.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Reviewing MR 8004 and see there is a 1 to 1 deletion to addition. One extra addition being phpcs.dist

Spot checking the files and nothing seemed super off based on our slack conversation.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Needs a rebase.

bbrala’s picture

Status: Needs work » Needs review

rebased.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Thank you! back to RTBC

catch’s picture

Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: -Major version only

Committed/pushed to 11.x and cherry-picked to 11.0.x, thanks!

We also need an MR for 10.4.x/10.3.x. I don't see any bc implication here because PHP treats these the same in terms of inheritance.

  • catch committed a376a5f4 on 11.0.x
    Issue #3427999 by andypost, Ayesh, bbrala: [PHP 8.4] Fix implicitly...

  • catch committed 06aeda30 on 11.x
    Issue #3427999 by andypost, Ayesh, bbrala: [PHP 8.4] Fix implicitly...

andypost’s picture

Version: 11.x-dev » 10.4.x-dev
Status: Patch (to be ported) » Needs review

Created MR for 10.4/10.3 there's extra files but sniffer and `phpcbf` produce reproducible result

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

10.4 version seems fine.

  • catch committed 24e197de on 10.3.x
    Issue #3427999 by andypost, Ayesh, bbrala: [PHP 8.4] Fix implicitly...

  • catch committed c71e641e on 10.4.x
    Issue #3427999 by andypost, Ayesh, bbrala: [PHP 8.4] Fix implicitly...
catch’s picture

Version: 10.4.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 10.4.x and cherry-picked to 10.3.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

solideogloria’s picture

Some were missed. I tried using Drupal 10.4.1 and PHP 8, and I get these errors:

Deprecated: Drupal\layout_builder\Form\BlockVisibilityForm::buildForm(): Implicitly marking parameter $section_storage as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/web/core/modules/layout_builder/src/Form/BlockVisibilityForm.php on line 115
Deprecated: Drupal\layout_builder\Form\ConfigureVisibilityForm::buildForm(): Implicitly marking parameter $section_storage as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/web/core/modules/layout_builder/src/Form/ConfigureVisibilityForm.php on line 172
Deprecated: Drupal\layout_builder\Form\DeleteVisibilityForm::buildForm(): Implicitly marking parameter $section_storage as nullable is deprecated, the explicit nullable type must be used instead in /var/www/html/web/core/modules/layout_builder/src/Form/DeleteVisibilityForm.php on line 90

solideogloria’s picture

Could a maintainer please reopen the issue? Or should I open a new one?

andypost’s picture

This files are not from core, please file new issue

EDIT it's from patch #2916876: Add visibility control conditions to blocks within Layout Builder

solideogloria’s picture

Ah, right you are. Thanks