Problem/Motivation

Not sure when this started but some recent runs of PHP 8.1rc5 and SQLite have resulted in failures in Drupal\Tests\field\Functional\Number\NumberFieldTest:

Unsilenced deprecation notices (2)

  1x: Implicit conversion from float 0.0001 to int loses precision
    1x in NumberFieldTest::testCreateNumberFloatField from Drupal\Tests\field\Functional\Number

  1x: Implicit conversion from float 0.1 to int loses precision
    1x in NumberFieldTest::testCreateNumberDecimalField from Drupal\Tests\field\Functional\Number

Steps to reproduce

See e.g. https://www.drupal.org/pift-ci-job/2244674 or https://www.drupal.org/pift-ci-job/2244720

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#5 3250743-5.patch2.35 KBalexpott
#4 3250743.will-fail-on-8.1.patch1.6 KBalexpott
#3 3250743.patch775 bytesalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

longwave created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Issue tags: +PHP 8.1
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Active » Needs review
FileSize
3250743.patch775 bytes

This was caused by the incorrect typehint being added by #3138078: [D9.3 beta - w/c Nov 8, 2021] Add a 'void' return typehint to custom assert* methods 

  $this->assertSetMinimumValue($field, 0.1);

and 

  $this->assertSetMinimumValue($field, 0.0001);

These are not ints.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Priority: Normal » Critical
FileSize
3250743.will-fail-on-8.1.patch1.6 KB

Okay so why is this not being reported on other 8.1 environments - I couldn't get it to fail locally at all when it definitely should. Well that's because we have an incorrect regex in \Drupal\Tests\Listeners\DeprecationListenerTrait::isDeprecationSkipped() added by #3232131: [Symfony 5] Symfony's DebugClassLoader triggers deprecation messages for missing return type hints, where there is no deprecation.

Here's proof.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
3250743-5.patch2.35 KB

And now for both of the patches together.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

The skipping of this deprecation is caused by https://regex101.com/r/C9t00b/1

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Title: [PHP 8.1] NumberFieldTest fails on SQLite » [PHP 8.1] NumberFieldTest fails

Ah... #3232131: [Symfony 5] Symfony's DebugClassLoader triggers deprecation messages for missing return type hints, where there is no deprecation has only been committed to 9.4.x so that explains why we think this is an sqlite thing. It's not at all. It's just that 9.3 tests PHP 8.1 on SQLite and fails... but on 9.4 / PHP 8.1 / MySQL it doesn't fail because of the regex mess.

So we need to commit #3 to 9.3.x and #5 to 9.4.x

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Status: Needs review » Reviewed & tested by the community

Both of the fixes look good, great debugging! Note:

So we need to commit #3 to 9.3.x and #5 to 9.4.x

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

RTBC+1, great work.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented

Would typehinting to float instead of removing the typehint for good work?

  • catch committed 62e7f6a on 9.4.x 
    Issue #3250743 by alexpott, longwave: [PHP 8.1] NumberFieldTest fails

  • catch committed d4bf8fb on 9.3.x 
    Issue #3250743 by alexpott, longwave: [PHP 8.1] NumberFieldTest fails
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed #5 to 9.4.x and #3 to 9.3.x. Good finds.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

@mondrake if we want to typehint to something actually string would be best as we're really using this value to put into forms.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented

#14 OK did not catch that

Status: Fixed » Closed (fixed)

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