Closed (fixed)
Project:
Simplenews
Version:
3.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
27 Dec 2020 at 15:31 UTC
Updated:
4 Dec 2021 at 10:24 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
tr commentedDo 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?
Comment #3
berdirThere 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.
Comment #4
tr commentedThe 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.
Comment #5
berdirThat fail is most likely the Drupal version and not PHP, that label possibly changed in 9.2
Comment #7
tr commentedYes, 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:
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.
Comment #8
tr commentedThat 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.
Comment #9
berdirmakes 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.
Comment #10
adamps commentedComment #12
adamps commented