Problem/Motivation

PHP deprecation notice:

PHP Deprecated:  a:5:{s:11:"deprecation";s:109:"Creation of dynamic property Drupal\Tests\redirect\Functional\RedirectUITest::$maximumRedirects is deprecated";s:5:"class";s:47:"Drupal\Tests\redirect\Functional\RedirectUITest";s:6:"method";s:16:"testRedirectLoop";s:15:"triggering_file";s:112:"/home/runner/work/acquia_cms/orca-build/docroot/modules/contrib/redirect/tests/src/Functional/RedirectUITest.php";s:11:"files_stack";a:7:{i:0;s:112:"/home/runner/work/acquia_cms/orca-build/docroot/modules/contrib/redirect/tests/src/Functional/RedirectUITest.php";i:1;s:77:"/home/runner/work/acquia_cms/orca-build/docroot/sites/simpletest/TestCase.php";i:2;s:77:"/home/runner/work/acquia_cms/orca-build/docroot/sites/simpletest/TestCase.php";i:3;s:91:"/home/runner/work/acquia_cms/orca-build/vendor/phpunit/phpunit/src/Framework/TestResult.php";i:4;s:77:"/home/runner/work/acquia_cms/orca-build/docroot/sites/simpletest/TestCase.php";i:5;s:19:"Standard input code";i:6;s:19:"Standard input code";}} in /home/runner/work/acquia_cms/orca-build/vendor/symfony/phpunit-bridge/Legacy/SymfonyTestsListenerTrait.php on line 284
CommentFileSizeAuthor
#20 3357833-redirect-php-8-2-compatibility-47-diff.patch7.38 KBandre.bonon
#11 Screenshot 2023-05-18 at 9.10.15 AM.png69.22 KBankitv18
#5 Screenshot 2023-05-15 at 10.42.32 AM.png574.66 KBrajeshreeputra

Issue fork redirect-3357833

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

vishalkhode created an issue. See original summary.

Rajeshreeputra made their first commit to this issue’s fork.

Rajeshreeputra opened merge request !47

rajeshreeputra’s picture

rajeshreeputra
he/him
Location Pune
commented
Issue tags: +ACMS2023
rajeshreeputra’s picture

rajeshreeputra
he/him
Location Pune
commented
Version: 8.x-1.8 » 8.x-1.x-dev
Status: Active » Needs review
StatusFileSize
new Screenshot 2023-05-15 at 10.42.32 AM.png574.66 KB

Test passing on local hence requesting review.
redirect tests

ankitv18’s picture

ankitv18 commented
Assigned: Unassigned » ankitv18
ankitv18’s picture

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

All changes are looking fine to me wrt Drupal version 10.0.7 and below tested with PHP 8.1 and 8.2 ~~ Hence marking this as RTBC.

Note: One test case is failing against for Drupal version 10.1.x with php 8.2

ankitv18’s picture

ankitv18 commented
Assigned: ankitv18 » Unassigned
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Reviewed & tested by the community » Needs work

Review posted. Try to avoid unrelated changes, the only really required change here that is in scope is defining the new properties.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

One more comment.

ankitv18’s picture

ankitv18 commented
Status: Needs work » Needs review
StatusFileSize
new Screenshot 2023-05-18 at 9.10.15 AM.png69.22 KB

Please review MR!47, FunctionalJavascriptTest is failing on Drupal 10.1 caused by bulk operation with default option as -select-
Attaching the screenshot for your reference.
Bulk operation

deepakkm’s picture

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

Ran test locally on php8.2 and are passing, also Changes looks good to me.
Hence moving into RTBC.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Assigned: Unassigned » kristen pol

Assigning to myself as I'm triaging all RTBC issues.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented

Been reviewing the comments and the commits and diffing things. Having the coding standard changes in it MR does make it harder to review which is something @Berdir mentioned above.

All permutations of tests have been run for 9.5/10.0/10.1 and pass which is good. I'm just trying to decide if this should be manually tested on all these version but that would be a bit labor intensive. Also, the info file has ^9.2 so I'll fire up tests for that at least to see what happens.

UPDATE: Can't run tests here below 9.5 so maybe I will at least test it on 9.2 manually.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented

Testing on 9.2 + PHP 8.0 went well.

UPDATE: Actually I was testing from an old project with directory name drupal92 but the site was actually 9.4 so it worked fine on 9.4 + PHP 8.0. I'll try on 9.2.

UPDATE2: Docker is misbehaving so I'm parking this for the moment.

Feel free to test on 9.2 + PHP 7.4!

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented

I've tested w/ PHP 8.2 and Core 10.1. Without the patch, when I went to re-save all views, I got warnings about dynamic properties. With the patch, I no longer get those warnings.

Tested using drush php:

$entities = \Drupal::entityTypeManager()->getStorage('view')->loadMultiple();
foreach ($entities as $entity) {$entity->save();}
kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented

Thanks @heddn. If this can be tested on 9.2 with PHP 7.4 then I'd feel comfortable merging it.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Assigned: kristen pol » Unassigned

Unassigning from me for now as I'm going to be on vacation for awhile and not sure when I could test this.

vipin.mittal18’s picture

vipin.mittal18
He/Him
Primary language English
Location Greater Noida, India
commented

Does anything block the stable release with PHP 8.2 deprecation fixes?

andre.bonon’s picture

andre.bonon
Primary language Portuguese, Brazil
commented
StatusFileSize
new 3357833-redirect-php-8-2-compatibility-47-diff.patch7.38 KB

Attaching patch to use in composer.json

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Reviewed & tested by the community » Fixed

Merged, thanks for your contributions.

Again, try to limit changes to what is actually required in an issue, that makes it far easier to review and get committed.

Status: Fixed » Closed (fixed)

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