Problem/Motivation

According to RFC escaping should be always provided

- https://wiki.php.net/rfc/deprecations_php_8_4#deprecate_proprietary_csv_...
- https://github.com/php/php-src/pull/15365

Steps to reproduce

Run test using PHP 8.4 or see https://git.drupalcode.org/issue/drupal-3427903/-/jobs/2855553

    /builds/issue/drupal-3427903/core/lib/Drupal/Core/Mail/Plugin/Mail/SymfonyMailer.php:130
    str_getcsv(): the $escape parameter must be provided as its default value
    will change

1)
/builds/issue/drupal-3427903/core/profiles/demo_umami/modules/demo_umami_content/src/InstallHelper.php:187
fgetcsv(): the $escape parameter must be provided as its default value will
change
Triggered by:
*
Drupal\FunctionalTests\Asset\AssetOptimizationTestUmami::testAssetAggregation
(18 times)
 
/builds/issue/drupal-3427903/core/tests/Drupal/FunctionalTests/Asset/AssetOptimizationTest.php:37
2)
/builds/issue/drupal-3427903/core/profiles/demo_umami/modules/demo_umami_content/src/InstallHelper.php:189
fgetcsv(): the $escape parameter must be provided as its default value will
change
Triggered by:
*
Drupal\FunctionalTests\Asset\AssetOptimizationTestUmami::testAssetAggregation
(172 times)
 
/builds/issue/drupal-3427903/core/tests/Drupal/FunctionalTests/Asset/AssetOptimizationTest.php:37
OK, but there were issues!
Tests: 1, Assertions: 213, Deprecations: 2.

Proposed resolution

fix usage to not throw deprecation by providing named argument for escape

Remaining tasks

- review
- commit

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3477324

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:

Comments

andypost created an issue. See original summary.

andypost’s picture

Title: Fix https://wiki.php.net/rfc/deprecations_php_8_4#deprecate_proprietary_csv_escaping_mechanism » Fix fgetcsv for PHP 8.4

andypost’s picture

Status: Active » Needs review

Looks like all arguments should be provided

andypost’s picture

Title: Fix fgetcsv for PHP 8.4 » Fix usage of str_getcsv() and fgetcsv() for PHP 8.4
Issue summary: View changes
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Seems straight forward enough and caused no failures

alexpott’s picture

Reading https://www.php.net/manual/en/function.fgetcsv.php

Warning
When escape is set to anything other than an empty string ("") it can result in CSV that is not compliant with » RFC 4180 or unable to survive a roundtrip through the PHP CSV functions. The default for escape is "\\" so it is recommended to set it to the empty string explicitly. The default value will change in a future version of PHP, no earlier than PHP 9.0.

I think we need a followup to change our default to an empty string or at least to discuss whether we should change this.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs followup

I think we should change the test and demo_umami usages here to the future default as this will not cause issues. We also should file the follow-up for the mailer plugin usages now. FWIW I'm pretty sure that changing escape to '' will be harmless but it seems worth doing in a separate issue.

andypost’s picture

Status: Needs work » Needs review
Issue tags: -Needs followup
Related issues: +#3482005: Change default escaping of str_getcsv() and fgetcsv() to empty string

Thanks Alex! Filed follow-up #3482005: Change default escaping of str_getcsv() and fgetcsv() to empty string

Applied suggestions for demo and test

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Believe feedback for this one has been addressed

andypost’s picture

I think it backportable to 10.4.x to keep compatibility

alexpott’s picture

Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 9cf58fe16c5 to 11.x and 8fc8778c8c5 to 11.1.x and 2e7842f8e51 to 11.0.x and 72b464b48bf to 10.5.x and 35d9969bc56 to 10.4.x and b5c897f4ab1 to 10.3.x. Thanks!

  • alexpott committed b5c897f4 on 10.3.x
    Issue #3477324 by andypost, alexpott: Fix usage of str_getcsv() and...

  • alexpott committed 35d9969b on 10.4.x
    Issue #3477324 by andypost, alexpott: Fix usage of str_getcsv() and...

  • alexpott committed 72b464b4 on 10.5.x
    Issue #3477324 by andypost, alexpott: Fix usage of str_getcsv() and...

  • alexpott committed 2e7842f8 on 11.0.x
    Issue #3477324 by andypost, alexpott: Fix usage of str_getcsv() and...

  • alexpott committed 8fc8778c on 11.1.x
    Issue #3477324 by andypost, alexpott: Fix usage of str_getcsv() and...

  • alexpott committed 9cf58fe1 on 11.x
    Issue #3477324 by andypost, alexpott: Fix usage of str_getcsv() and...

Status: Fixed » Closed (fixed)

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