Problem/Motivation

\Drupal\views_ui\Form\Ajax\ViewsFormBase::getForm() relies on very odd PHP behaviour that is fixed in PHP 8.

This is happening because of this behaviour change in PHP 8.0 - https://3v4l.org/TJ2cb.

This change in behaviour in PHP 8 is explained with this RFC: https://wiki.php.net/rfc/string_to_number_comparison. TL;DR, PHP 8 has adopted changes to how string to integer comparisons should be handled and no longer type juggling a string to a numeric equivalent before comparison.

Steps to reproduce

if (isset($view->form_cache) && $view->form_cache['key'] != $key) {
to
if (isset($view->form_cache) && $view->form_cache['key'] !== $key) {
and run core/modules/views_ui/tests/src/Functional/FilterUITest.php

see https://dispatcher.drupalci.org/job/drupal_patches/63999/

Proposed resolution

Don't duplicate the use $key and change to !==

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#6 3177590-6.patch2.13 KBalexpott
#6 2-6-interdiff.txt1.85 KBalexpott
#2 3177590-2.patch1.26 KBalexpott
#2 3177590-2-test-only.patch733 bytesalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Issue summary: View changes
Status: Active » Needs review
Related issues: +#3109885: [meta] Ensure compatibility of Drupal 9 with PHP 8.0 (as it evolves)
FileSize
3177590-2-test-only.patch733 bytes
3177590-2.patch1.26 KB

Here's a fix. This is happening because of this behaviour change in PHP 8.0 - https://3v4l.org/TJ2cb

The last submitted patch, 2: 3177590-2-test-only.patch, failed testing. View results

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented 
+++ b/core/modules/views_ui/src/Form/Ajax/ViewsFormBase.php
@@ -118,7 +118,7 @@ public function getForm(ViewEntityInterface $view, $display_id, $js) {
     $key = $form_state->get('form_key');
...
+    if (isset($view->form_cache) && $view->form_cache['key'] !== $key) {

Would it be better to also rename $key as $form_key?

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Related issues: +#1530652: [META] Switch from == to === to prevent mistakes and make brute force password attacks harder
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
2-6-interdiff.txt1.85 KB
3177590-6.patch2.13 KB

Re #4 sure - why not.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented
Issue summary: View changes

Modified IS to explain the change in PHP 8 with more details and link to RFC.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented
Status: Needs review » Reviewed & tested by the community

That said, this does LGTM.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented

RTBC +1

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented

Late to the party, but wow that was an interesting one. Nice find, nice fix.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: 3177590-6.patch, failed testing. View results

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs work » Reviewed & tested by the community

Bot flux

Drupal\BuildTests\Framework\Tests\BuildTestTest::testPortMany
RuntimeException: Unable to start the web server.

  • catch committed bf1dd77 on 9.2.x 
    Issue #3177590 by alexpott, longwave, hussainweb: ViewsFormBase::getForm...

  • catch committed 668fb6b on 9.1.x 
    Issue #3177590 by alexpott, longwave, hussainweb: ViewsFormBase::getForm...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed bf1dd77 and pushed to 9.2.x. Thanks!
Cherry-picked to 9.1.x too.

Status: Fixed » Closed (fixed)

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