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
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:
- 3477324-fix-fgetcsv-for
changes, plain diff MR !9649
Comments
Comment #2
andypostComment #4
andypostLooks like all arguments should be provided
Comment #5
andypostClosed as duplicate #3477307: Fix str_getcsv() usage for PHP 8.4
Comment #6
smustgrave commentedSeems straight forward enough and caused no failures
Comment #7
alexpottReading https://www.php.net/manual/en/function.fgetcsv.php
I think we need a followup to change our default to an empty string or at least to discuss whether we should change this.
Comment #8
alexpottI 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.
Comment #9
andypostThanks Alex! Filed follow-up #3482005: Change default escaping of str_getcsv() and fgetcsv() to empty string
Applied suggestions for demo and test
Comment #10
smustgrave commentedBelieve feedback for this one has been addressed
Comment #11
andypostI think it backportable to 10.4.x to keep compatibility
Comment #12
alexpottCommitted 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!