Problem/Motivation

These mostly refer to PHP native classes, could be fixed for PHP 8.1.

Proposed resolution

Add the relevant typehints and remove entry from .deprecation-ignore.txt

Issue fork drupal-3425337

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:

About issue forks

Comments

mondrake created an issue. See original summary.

mondrake opened merge request !6887

quietone’s picture

quietone commented
Title: [11.x] Replace root namespace native DebugClassLoader forward compatibility warnings » Replace root namespace native DebugClassLoader forward compatibility warnings
mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: Replace root namespace native DebugClassLoader forward compatibility warnings » Fix root namespace native DebugClassLoader forward compatibility warnings
Status: Active » Needs review
mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes
mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: Fix root namespace native DebugClassLoader forward compatibility warnings » Fix root namespace classes DebugClassLoader forward compatibility warnings
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Typehints make sense. Removing from deprecation-ignore didn't cause failures, which I find is a good sign for these kind of tickets.

  • longwave committed ff7c5de3 on 11.x 
    Issue #3425337 by mondrake: Fix root namespace classes DebugClassLoader...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

Nice to see these finally all have their correct types.

Committed ff7c5de and pushed to 11.x. Thanks!

longwave closed merge request !6887

catch’s picture

catch
he/him
Primary language English
commented
Status: Fixed » Needs work

This is causing unit tests to fail with a deprecation warning https://git.drupalcode.org/project/drupal/-/jobs/978330 Not sure how the pipeline here passed, maybe a cross-commit?

  • catch committed 03b8070a on 11.x 
    Revert "Issue #3425337 by mondrake: Fix root namespace classes...
catch’s picture

catch
he/him
Primary language English
commented

Reverted - let's try again.

mondrake reopened merge request !6887

mondrake’s picture

mondrake
Location 🇮🇹
commented

Deprecation of the DebugClassLoader are very fragile... also noticed that in #3417066: Upgrade PHPUnit to 10, drop Symfony PHPUnit-bridge dependency, where by filling the gap of symfony-bridge I found more errors reported. The strange thing here is that this appears to have dfferent behaviour in MR testing vs branch testing...

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Reviewed & tested by the community

I rebased and checked, the error reported in #11 was fixed in the MR already, so 🤷

  • longwave committed ea9cc753 on 11.x 
    Issue #3425337 by mondrake: Fix root namespace classes DebugClassLoader...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

Maybe I missed it somehow when I committed?! Trying again...

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Should we have another issue to remove all remaining #[ReturnTypeWillChange] in 11.x?

mondrake’s picture

mondrake
Location 🇮🇹
commented

Indeed, some seem not to have been reported... I will check with #3417066: Upgrade PHPUnit to 10, drop Symfony PHPUnit-bridge dependency. Sure we need a followup.

catch’s picture

catch
he/him
Primary language English
commented

If I run this test locally:

../vendor/bin/phpunit modules/file/tests/src/Functional/FileAddPermissionsUpdateTest.php

I get

1) Drupal\Tests\file\Functional\FileAddPermissionsUpdateTest::testUpdate
Exception: Deprecated function: Return type of Drupal\Component\Annotation\Doctrine\StaticReflectionClass::getStaticProperties(): ?array should either be compatible with ReflectionClass::getStaticProperties(): array, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice
include()() (Line: 32)
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented

@catch just found this too
I found this issue because for some reason gitlabci started to test with 8.3 when we were testing the Automatic Updates module with core 11.x

In the commit

    -  #[\ReturnTypeWillChange]
    - public function getStaticProperties()
    + public function getStaticProperties(): ?array

I think this will cause an error on PHP 8.3 because
ReflectionClass::getStaticProperties(): ?array was changed to ReflectionClass::getStaticProperties(): array so removing the `ReturnTypeWillChange` actually makes this not compatible with PHP 8.3's return type. I couldn't tell if core itself or this merge request was tested on PHP 8.3(still getting use to the gitlab pipelines UI) but looks like 11.x should support PHP 8.3, correct?

mondrake’s picture

mondrake
Location 🇮🇹
commented

Notice is right, the signature is public ReflectionClass::getStaticProperties(): array, https://www.php.net/manual/it/reflectionclass.getstaticproperties.php

Will fix it in #3425537: Fix remaining methods with #[\ReturnTypeWillChange] attribute that is still to be committed.

andypost’s picture

andypost
he/him
Primary language Russian
commented

Maybe it's time to enable 8.3 as default for 11.x?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Yep we already agreed #3330874: [11.x] [policy] Require PHP 8.3 for Drupal 11 we should change this in 11.x now.

mondrake’s picture

mondrake
Location 🇮🇹
commented

I think this needs to be temporarily reverted, and #3400984: Make PHP 8.3 the default environment for gitlab CI runs done first. BTW there’s a sorting test that fails on PHP 8.3 and I think it’s unrelated.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Fixed » Needs work

@mondrake was watching your test runs on the other issue and have come to the same conclusion.

  • longwave committed c5c85f18 on 11.x 
    Revert "Issue #3425337 by mondrake: Fix root namespace classes...
mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Reviewed & tested by the community

Seems OK now after the PHP 8.3 bump in CI

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

Third time lucky. Now we are testing on PHP 8.3 I think this is finally OK!

Committed f330fa3 and pushed to 11.x. Thanks!

  • longwave committed f330fa37 on 11.x 
    Issue #3425337 by mondrake: Fix root namespace classes DebugClassLoader...

longwave closed merge request !6887

  • longwave committed bd3e71a8 on 11.x 
    Issue #3425337 by mondrake: Fix root namespace classes DebugClassLoader...

Status: Fixed » Closed (fixed)

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