Problem/Motivation

Add module compatibility for PHP 8 on Drupal 9.1

Comments

Universaldenker created an issue. See original summary.

tr’s picture

Status: Active » Postponed (maintainer needs more info)

Do you know that it *doesn't* work in PHP 8? If so, what are the problems? And can you make a patch to fix any problems you found?

berdir’s picture

There are some test fails on https://www.drupal.org/pift-ci-job/2083264, but I'm not sure if those are just random fails or a Drupal 9 problem or so, don't see any explicitly PHP 8 related failures. so leaving the status as it is for now.

tr’s picture

Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new1014 bytes

The two fails I see in PHP 8.0 are because the button on the config form at /user/{%user}/cancel shows up with the default "Confirm" label instead of the "Cancel account" label. As this is a core form for user cancellation, the only obvious thing I can think of is that this may be related to using 'classy' as the default theme for testing this module. 'classy' was added as the default theme in #3042666: Drupal 9 Deprecated Code Report - I'm not sure why this was picked instead of 'stark', but attached is a patch to see if this error goes away when using the recommended 'stark' instead.

Artifact from https://www.drupal.org/pift-ci-job/2236343 (PHP 7.3 & MySQL 5.7, Drupal 9.2.0)
https://dispatcher.drupalci.org/job/drupal8_contrib_patches/87634/artifa...
Confirm form shows "Cancel account" button, so the test that looks for this button passes.

Artifact from https://www.drupal.org/pift-ci-job/2236344 (PHP 8.0 & MySQL 5.7, Drupal 9.3.x)
https://dispatcher.drupalci.org/job/drupal8_contrib_patches/87635/artifa...
Confirm form looks the same as above, but this time it is labelled "Confirm" instead of "Cancel account", so the test that looks for this button fails.

berdir’s picture

That fail is most likely the Drupal version and not PHP, that label possibly changed in 9.2

Status: Needs review » Needs work

The last submitted patch, 4: 3190008-4-use-stark-instead-of-classy.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tr’s picture

Version: 3.0.0-alpha1 » 3.x-dev
Category: Feature request » Task
Status: Needs work » Needs review
StatusFileSize
new1.66 KB

Yes, you appear to be correct. I re-ran the PHP 8.0 test with core 9.2.0 and it passed. See https://www.drupal.org/pift-ci-job/2236869

The core change was made in issue #3199972: Improve user interface text on the account cancellation screen, which changed the cancel form button label like this:

   /**
    * {@inheritdoc}
    */
   public function getConfirmText() {
-    return $this->t('Cancel account');
+    return $this->t('Confirm');
   }

I personally think that's a step backwards in usability, but whatever. The bigger problem is that a string change like this makes it awkward to write tests that work in all supported versions of core simultaneously.

Here's a new patch to fix that part of the test. This patch still uses 'classy' because the above test results show there are new problems that arise when we switch to 'stark'.

It would be useful to add a new testing configuration for this module to test weekly against PHP 8 and D9.3.x (latest development) so that we would get timely warning when core changes affect this module.

tr’s picture

StatusFileSize
new1.66 KB
new383 bytes

That worked and shows that the patch makes this module's tests pass on PHP 8 for all supported D9 versions.

Here's a new patch to correct the coding standards error (extra blank line) introduced in #7.

berdir’s picture

Status: Needs review » Reviewed & tested by the community

makes sense. Now that D8 is no longer supported, I suppose we can change the tests to run against latest stable D9 for patches/MRs and latest dev branch for a weekly test, yes. Can also use PHP 8 for that.

adamps’s picture

Title: Compatibility with PHP 8 on Drupal 9.1 » Tests fail with Drupal 9.3

  • AdamPS committed f640b4f on 3.x authored by TR
    Issue #3190008 by TR, Berdir, AdamPS: Tests fail with Drupal 9.3
    
adamps’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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