Make CSV deprecation less annoying to deal with #15569

Girgias · 2024-08-24T14:24:49Z

@nyamsprod I've changed the behaviour of the deprecation to be more sensible in that it requires people to provide the $escape parameter.

@cmb69 you might want to have a look too as the original author of https://wiki.php.net/rfc/kill-csv-escaping

cmb69 · 2024-08-24T17:49:12Z

I think this makes sense; in the original RFC, I probably should better have written "using" instead of "passing". However, it is almost impossible to get rid of the parameter at some point, since the $eol parameter had been added to fputcsv(). A similar change to the signature of imagepolygon() and friends was only possible due to all parameters have been mandatory anyway.

Girgias · 2024-08-24T18:28:52Z

I think it is possible to force usage of a named parameter by checking if the value is set and ZEND_ARG_NUMs is less than its position.

I think the main objective should be to change the default value so that people don't get bitten when writing new code.

cmb69 · 2024-08-24T18:45:27Z

I think the main objective should be to change the default value so that people don't get bitten when writing new code.

Yeah, sure! But it would also be nice to eventually get rid of the implementation of the escape parameter. But that can't happen anytime soon, anyway.

nyamsprod · 2024-08-26T09:43:13Z

@Girgias thanks for the update and the change so if I understand correctly the deprecation will only happens if the user explicitly provides an escape parameter other than the empty string

So if no escape parameter is provided or if the default is provided no deprecation should happen correct ?

$obj = new SplTempFileObject();
$obj->fwrite($csv);
$obj->setCsvControl(',', escape: "|");  // this should trigger the deprecation notce
$obj->setCsvControl(',', escape: "\\"); // this should not

Girgias · 2024-08-26T11:28:27Z

@Girgias thanks for the update and the change so if I understand correctly the deprecation will only happens if the user explicitly provides an escape parameter other than the empty string

So if no escape parameter is provided or if the default is provided no deprecation should happen correct ?
$obj = new SplTempFileObject();
$obj->fwrite($csv);
$obj->setCsvControl(',', escape: "|");  // this should trigger the deprecation notce
$obj->setCsvControl(',', escape: "\\"); // this should not 

No, a deprecation is triggered only if you do not specify the $escape argument, either positionally or with a named argument.

The primary objective is to change the default of the $escape parameter from "\\" to the empty string "".
So this changes would still allow people to use their custom escape argument with PHP's proprietary escape mechanism but they need to explicitly request it.

nyamsprod · 2024-08-26T11:36:42Z

OK so if I rewrite my example:

$obj = new SplTempFileObject();
$obj->setCsvControl(',', escape: "|");  // this should not trigger the deprecation notice
$obj->setCsvControl(',', escape: "\\"); // this should not trigger the deprecation notice
$obj->setCsvControl(','); // this SHOULD trigger the deprecation notice

Girgias · 2024-08-26T12:05:11Z

OK so if I rewrite my example:

$obj = new SplTempFileObject();
$obj->setCsvControl(',', escape: "|");  // this should not trigger the deprecation notice
$obj->setCsvControl(',', escape: "\\"); // this should not trigger the deprecation notice
$obj->setCsvControl(','); // this SHOULD trigger the deprecation notice

This would be correct yes, I hope this makes it less annoying. :)

nyamsprod · 2024-08-26T12:18:43Z

for league/csv this makes for a big improvement because the escape character is always set so there will be no deprecation notices at all but for user of the "native" PHP feature this will make it less annoying but I will still keep advocating for the empty string usage 👍🏾 because that's the way to go about it.

Thanks for the improvement

jrfnl · 2024-09-17T03:46:53Z

OK so if I rewrite my example:

$obj = new SplTempFileObject();
$obj->setCsvControl(',', escape: "|");  // this should not trigger the deprecation notice
$obj->setCsvControl(',', escape: "\\"); // this should not trigger the deprecation notice
$obj->setCsvControl(','); // this SHOULD trigger the deprecation notice

This would be correct yes, I hope this makes it less annoying. :)

I'm not sure that it does and it also is no longer in line with the RFC which was voted on.

It now requires everyone to start passing the parameter, effectively making the function signature invalid according to PHP itself, as there is now basically a required parameter after two optional parameters.

If we look at prior art, there have been various other functions where the default value of an optional parameter was changed in a PHP minor. To name a few:

default character set for htmlspecialchars(), htmlentities() and html_entity_decode() in PHP 5.4.
default character set for $encoding for various Iconv and Mbstring functions in PHP 5.6.
default value for the $variant parameter for idn_to_*() functions in PHP 7.4.
default value for the $flags parameters for htmlspecialchars(), htmlentities(), htmlspecialchars_decode(), html_entity_decode() and get_html_translation_table() in PHP 8.1.

None of these changes first made the optional parameter (soft) "required" via a deprecation notice. The default value just changed and people had the choice to pass the parameter if they wanted to enforce either the old or the new behaviour and to not pass the parameter if they didn't care or if the change didn't affect their use-case.

This two-step approach just makes things more complicated.

If I'm honest, I think it would be better to withdraw this change from PHP 8.4 and bring it up for vote again for PHP 8.5 or 9.0, but then to change the default value without a deprecation notice.

nyamsprod · 2024-09-17T06:11:20Z

If I'm honest, I think it would be better to withdraw this change from PHP 8.4 and bring it up for vote again for PHP 8.5 or 9.0, but then to change the default value without a deprecation notice.

@jrfnl I tend to agree with you on that one because the behaviour voted on in the RFC is highly problematic. While the "new" behaviour is much saner I am also in the camp of those who think that changing the parameter default in the next major PHP version without deprecation is the best solution as long as the escape parameter is still around for BC compatibility.

cmb69 · 2024-09-17T10:38:11Z

I think, the wording of the RFC was a bit unfortunate (since based on my prior withdrawn RFC). The problem is not really passing a non-empty string, but using a non-empty string; i.e. the problem is to rely on the proprietary escaping mechanism. As such, the behavior prior to this patch is (almost) useless: those relying on the default will not be notified (although their reliance on the default is likely unconcious), but those deliberately using an $escape which is not contained in their data (e .g. \0) to circumvent the proprietary escaping will see a deprecation notice.

What about silently changing the default? I think this is somewhat problematic, since CSV may well be used to store persistent data, and if the default changes, that may cause issues when reading CSV written by older PHP versions without any notice about why that happens. Although I have to admit that even writing/reading with the same PHP version may cause issues unless the developer is aware of the proprietary escaping mechanism.

So, in my opinion, we should either stick with what we have now, or completely drop #15362. But this should be discussed on the internals mailing. I'm afraid that many who voted on the RFC are not aware of this PR, but would notice the change when discussed on internals.

Girgias · 2024-09-18T15:21:04Z

OK so if I rewrite my example:
$obj = new SplTempFileObject();
$obj->setCsvControl(',', escape: "|");  // this should not trigger the deprecation notice
$obj->setCsvControl(',', escape: "\\"); // this should not trigger the deprecation notice
$obj->setCsvControl(','); // this SHOULD trigger the deprecation notice
This would be correct yes, I hope this makes it less annoying. :)
I'm not sure that it does and it also is no longer in line with the RFC which was voted on.

It now requires everyone to start passing the parameter, effectively making the function signature invalid according to PHP itself, as there is now basically a required parameter after two optional parameters.

If we look at prior art, there have been various other functions where the default value of an optional parameter was changed in a PHP minor. To name a few:
* default character set for `htmlspecialchars()`, `htmlentities()` and `html_entity_decode()` in PHP 5.4.

* default character set for `$encoding` for various Iconv and Mbstring functions in PHP 5.6.

* default value for the `$variant` parameter for `idn_to_*()` functions in PHP 7.4.

* default value for the `$flags` parameters for `htmlspecialchars()`, `htmlentities()`, `htmlspecialchars_decode()`, `html_entity_decode()` and `get_html_translation_table()` in PHP 8.1.
None of these changes first made the optional parameter (soft) "required" via a deprecation notice. The default value just changed and people had the choice to pass the parameter if they wanted to enforce either the old or the new behaviour and to not pass the parameter if they didn't care or if the change didn't affect their use-case.

This two-step approach just makes things more complicated.

If I'm honest, I think it would be better to withdraw this change from PHP 8.4 and bring it up for vote again for PHP 8.5 or 9.0, but then to change the default value without a deprecation notice.

Arguably, the previous implementation was wrong.
As the RFC states:

[...] is to deprecate passing a non-empty string to the $escape parameter to all the CSV related functions, effectively disabling our problematic escaping mechanism.

But a default value is still passed to the function, and because the default value is not an empty string it should have, to be correctly implemented, emit a deprecation notice.

Yes this makes this previously "optional" parameter required, but there is no other way of achieving this, and this argument was used to shut down the RFC from @cmb69 5 years ago, however this still causes issues today and bites people constantly.

If you want to argue on internals about this, I cannot stop you, but my previous implementation was wrong anyway.

jrfnl · 2024-09-18T18:22:45Z

I'm not sure that it does and it also is no longer in line with the RFC which was voted on.
It now requires everyone to start passing the parameter, effectively making the function signature invalid according to PHP itself, as there is now basically a required parameter after two optional parameters.

Arguably, the previous implementation was wrong. As the RFC states:

[...] is to deprecate passing a non-empty string to the $escape parameter to all the CSV related functions, effectively disabling our problematic escaping mechanism.

But a default value is still passed to the function, and because the default value is not an empty string it should have, to be correctly implemented, emit a deprecation notice.

Yes this makes this previously "optional" parameter required, but there is no other way of achieving this, and this argument was used to shut down the RFC from @cmb69 5 years ago, however this still causes issues today and bites people constantly.

If you want to argue on internals about this, I cannot stop you, but my previous implementation was wrong anyway.

To be honest, I shouldn't have to bring this up on internals. You should though, as you discovered that what was proposed was not a good way forward and you have now implemented a different solution than what was in the RFC.

cmb69 · 2024-09-19T11:18:42Z

I've written to the list for clarification.

jrfnl · 2024-09-19T21:31:40Z

Thanks @cmb69!

Girgias · 2024-09-20T12:24:39Z

I'm not sure that it does and it also is no longer in line with the RFC which was voted on.
It now requires everyone to start passing the parameter, effectively making the function signature invalid according to PHP itself, as there is now basically a required parameter after two optional parameters.

Arguably, the previous implementation was wrong. As the RFC states:

[...] is to deprecate passing a non-empty string to the $escape parameter to all the CSV related functions, effectively disabling our problematic escaping mechanism.

But a default value is still passed to the function, and because the default value is not an empty string it should have, to be correctly implemented, emit a deprecation notice.
Yes this makes this previously "optional" parameter required, but there is no other way of achieving this, and this argument was used to shut down the RFC from @cmb69 5 years ago, however this still causes issues today and bites people constantly.
If you want to argue on internals about this, I cannot stop you, but my previous implementation was wrong anyway.

To be honest, I shouldn't have to bring this up on internals. You should though, as you discovered that what was proposed was not a good way forward and you have now implemented a different solution than what was in the RFC.

When I implemented initially, I thought it also threw a deprecation on default values of the parameter.
The current implementation of those functions is, questionable.

bukka · 2024-09-23T21:04:00Z

So I think this should be reverted as it doesn't match what is in RFC and it's really getting close a real feature freeze.

Also any features (even the RFC implementation) during beta should be approved by (get blessing from) RM (see https://github.com/php/policies/blob/main/release-process.rst#beta-releases ).

With [1] the 5th parameter `$escape` of `fgetcsv()` must be provided either positional or using named arguments or a E_DEPRECATED will be emitted since `PHP 8.4.0 RC1` [2]. This change provide now all five parameter for `fgetcsv()` calls and thus using the positional approach to allow easier backporting to older TYPO3 version where named arguements are not usable. [1] php/php-src#15569 [2] https://github.com/php/php-src/blob/ebee8df27ed/UPGRADING#L617-L622 Releases: main, 8, 7

With [1] the 5th parameter `$escape` of `fgetcsv()` must be provided either positional or using named arguments or a E_DEPRECATED will be emitted since `PHP 8.4.0 RC1` [2]. This change provide now all five parameter for `fgetcsv()` calls and thus using the positional approach to allow easier backporting to older TYPO3 version where named arguements are not usable. [1] php/php-src#15569 [2] https://github.com/php/php-src/blob/ebee8df27ed/UPGRADING#L617-L622 Releases: main, 8, 7, 6

Note that this change addresses three different issues, which need to be done in one step, otherwise none of isolated patches would get a +1 CI verification: * Alignment for changed `$escape` parameter handling of CSV related methods, which must be avoided with PHP versions below 8.4.0-beta5, but is required as of PHP 8.4.0-RC1. * This `$escape` issue needs to be addressed directly in the `typo3/testing-framework` and thus needs a raised dependency directly. * PHP 8.4.0.0-RC1 also deprecated the `E_STRICT` constant, which needs to be addressed directly. With [1] the `$escape` parameter for the following method calls * `str_getcsv()` * `fputcsv()` * `fgetcsv()` must be provided as a 1 character long value. Omitting and using the default value will emit a PHP deprecation [2] warning as of PHP 8.4.0-RC1, for example: str_getcsv(): the $escape parameter must be provided as its default value will change To mitigate this, PHP recommends following: It must be passed explicitly either positionally or via named arguments. This change adjusts all occurences (function calls) and ensures that the `$escape` parameter is explicitly provided via specifying all parameters up to that position and not using a `named arguments` approach for easier backporting. The TYPO3 testing framework also needs to be aligned to mitigate `fgetcsv()` issues, and is raised in the same step - otherwise none of these changes would get a green CI pipeline run. The following testing-framework updates are adjusted: * main: simple update `main` pointer * 12.4: Raise to tag `8.2.2` * 11.5: Raise to tag `6.16.10` Used command(s): > composer require --dev "typo3/testing-framework":"^8.2.2" Additionally, PHP 8.4.0-RC1 deprecated the `E_STRICT` constant, which now emits a E_DEPRECATED which leads to failed CI tests. This change introduces a core internal constant with the same integer value, and replaces usages with this constant to mitigate the E_DEPRECATION. This constant can then be dropped with TYPO3 v14, preventing a breaking change at this time. [3][4] [1] php/php-src#15569 [2] https://github.com/php/php-src/blob/ebee8df27ed/UPGRADING#L617-L622 [3] https://github.com/php/php-src/blob/ebee8df27edf7/UPGRADING#L47-L49 [4] php/php-src#13053 Resolves: #105155 Releases: main, 12.4, 11.5 Change-Id: Ie8b7d46eeb75ba6e32c0e8f6e7e947775083cc15 Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/86376 Tested-by: core-ci <[email protected]> Tested-by: Stefan Bürk <[email protected]> Reviewed-by: Garvin Hicking <[email protected]> Reviewed-by: Stefan Bürk <[email protected]> Tested-by: Garvin Hicking <[email protected]>

Note that this change addresses three different issues, which need to be done in one step, otherwise none of isolated patches would get a +1 CI verification: * Alignment for changed `$escape` parameter handling of CSV related methods, which must be avoided with PHP versions below 8.4.0-beta5, but is required as of PHP 8.4.0-RC1. * This `$escape` issue needs to be addressed directly in the `typo3/testing-framework` and thus needs a raised dependency directly. * PHP 8.4.0.0-RC1 also deprecated the `E_STRICT` constant, which needs to be addressed directly. With [1] the `$escape` parameter for the following method calls * `str_getcsv()` * `fputcsv()` * `fgetcsv()` must be provided as a 1 character long value. Omitting and using the default value will emit a PHP deprecation [2] warning as of PHP 8.4.0-RC1, for example: str_getcsv(): the $escape parameter must be provided as its default value will change To mitigate this, PHP recommends following: It must be passed explicitly either positionally or via named arguments. This change adjusts all occurences (function calls) and ensures that the `$escape` parameter is explicitly provided via specifying all parameters up to that position and not using a `named arguments` approach for easier backporting. The TYPO3 testing framework also needs to be aligned to mitigate `fgetcsv()` issues, and is raised in the same step - otherwise none of these changes would get a green CI pipeline run. The following testing-framework updates are adjusted: * main: simple update `main` pointer * 12.4: Raise to tag `8.2.2` * 11.5: Raise to tag `6.16.10` Used command(s): > composer update "typo3/testing-framework" Additionally, PHP 8.4.0-RC1 deprecated the `E_STRICT` constant, which now emits a E_DEPRECATED which leads to failed CI tests. This change introduces a core internal constant with the same integer value, and replaces usages with this constant to mitigate the E_DEPRECATION. This constant can then be dropped with TYPO3 v14, preventing a breaking change at this time. [3][4] [1] php/php-src#15569 [2] https://github.com/php/php-src/blob/ebee8df27ed/UPGRADING#L617-L622 [3] https://github.com/php/php-src/blob/ebee8df27edf7/UPGRADING#L47-L49 [4] php/php-src#13053 Resolves: #105155 Releases: main, 12.4, 11.5 Change-Id: Ie8b7d46eeb75ba6e32c0e8f6e7e947775083cc15 Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/86371 Tested-by: Garvin Hicking <[email protected]> Reviewed-by: Stefan Bürk <[email protected]> Tested-by: Anja Leichsenring <[email protected]> Reviewed-by: Anja Leichsenring <[email protected]> Tested-by: core-ci <[email protected]> Tested-by: Stefan Bürk <[email protected]> Reviewed-by: Garvin Hicking <[email protected]>

Note that this change addresses three different issues, which need to be done in one step, otherwise none of isolated patches would get a +1 CI verification: * Alignment for changed `$escape` parameter handling of CSV related methods, which must be avoided with PHP versions below 8.4.0-beta5, but is required as of PHP 8.4.0-RC1. * This `$escape` issue needs to be addressed directly in the `typo3/testing-framework` and thus needs a raised dependency directly. * PHP 8.4.0.0-RC1 also deprecated the `E_STRICT` constant, which needs to be addressed directly. With [1] the `$escape` parameter for the following method calls * `str_getcsv()` * `fputcsv()` * `fgetcsv()` must be provided as a 1 character long value. Omitting and using the default value will emit a PHP deprecation [2] warning as of PHP 8.4.0-RC1, for example: str_getcsv(): the $escape parameter must be provided as its default value will change To mitigate this, PHP recommends following: It must be passed explicitly either positionally or via named arguments. This change adjusts all occurences (function calls) and ensures that the `$escape` parameter is explicitly provided via specifying all parameters up to that position and not using a `named arguments` approach for easier backporting. The TYPO3 testing framework also needs to be aligned to mitigate `fgetcsv()` issues, and is raised in the same step - otherwise none of these changes would get a green CI pipeline run. The following testing-framework updates are adjusted: * main: simple update `main` pointer * 12.4: Raise to tag `8.2.2` * 11.5: Raise to tag `6.16.10` Used command(s): > composer require --dev "typo3/testing-framework":"^6.16.10" Additionally, PHP 8.4.0-RC1 deprecated the `E_STRICT` constant, which now emits a E_DEPRECATED which leads to failed CI tests. This change introduces a core internal constant with the same integer value, and replaces usages with this constant to mitigate the E_DEPRECATION. This constant can then be dropped with TYPO3 v14, preventing a breaking change at this time. [3][4] [1] php/php-src#15569 [2] https://github.com/php/php-src/blob/ebee8df27ed/UPGRADING#L617-L622 [3] https://github.com/php/php-src/blob/ebee8df27edf7/UPGRADING#L47-L49 [4] php/php-src#13053 Resolves: #105155 Releases: main, 12.4, 11.5 Change-Id: Ie8b7d46eeb75ba6e32c0e8f6e7e947775083cc15 Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/86377 Tested-by: Stefan Bürk <[email protected]> Reviewed-by: Garvin Hicking <[email protected]> Tested-by: core-ci <[email protected]> Reviewed-by: Stefan Bürk <[email protected]> Tested-by: Garvin Hicking <[email protected]>

Note that this change addresses three different issues, which need to be done in one step, otherwise none of isolated patches would get a +1 CI verification: * Alignment for changed `$escape` parameter handling of CSV related methods, which must be avoided with PHP versions below 8.4.0-beta5, but is required as of PHP 8.4.0-RC1. * This `$escape` issue needs to be addressed directly in the `typo3/testing-framework` and thus needs a raised dependency directly. * PHP 8.4.0.0-RC1 also deprecated the `E_STRICT` constant, which needs to be addressed directly. With [1] the `$escape` parameter for the following method calls * `str_getcsv()` * `fputcsv()` * `fgetcsv()` must be provided as a 1 character long value. Omitting and using the default value will emit a PHP deprecation [2] warning as of PHP 8.4.0-RC1, for example: str_getcsv(): the $escape parameter must be provided as its default value will change To mitigate this, PHP recommends following: It must be passed explicitly either positionally or via named arguments. This change adjusts all occurences (function calls) and ensures that the `$escape` parameter is explicitly provided via specifying all parameters up to that position and not using a `named arguments` approach for easier backporting. The TYPO3 testing framework also needs to be aligned to mitigate `fgetcsv()` issues, and is raised in the same step - otherwise none of these changes would get a green CI pipeline run. The following testing-framework updates are adjusted: * main: simple update `main` pointer * 12.4: Raise to tag `8.2.2` * 11.5: Raise to tag `6.16.10` Used command(s): > composer require --dev "typo3/testing-framework":"^8.2.2" Additionally, PHP 8.4.0-RC1 deprecated the `E_STRICT` constant, which now emits a E_DEPRECATED which leads to failed CI tests. This change introduces a core internal constant with the same integer value, and replaces usages with this constant to mitigate the E_DEPRECATION. This constant can then be dropped with TYPO3 v14, preventing a breaking change at this time. [3][4] [1] php/php-src#15569 [2] https://github.com/php/php-src/blob/ebee8df27ed/UPGRADING#L617-L622 [3] https://github.com/php/php-src/blob/ebee8df27edf7/UPGRADING#L47-L49 [4] php/php-src#13053 Resolves: #105155 Releases: main, 12.4, 11.5 Change-Id: Ie8b7d46eeb75ba6e32c0e8f6e7e947775083cc15 Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/86376 Tested-by: core-ci <[email protected]> Tested-by: Stefan Bürk <[email protected]> Reviewed-by: Garvin Hicking <[email protected]> Reviewed-by: Stefan Bürk <[email protected]> Tested-by: Garvin Hicking <[email protected]>

Note that this change addresses three different issues, which need to be done in one step, otherwise none of isolated patches would get a +1 CI verification: * Alignment for changed `$escape` parameter handling of CSV related methods, which must be avoided with PHP versions below 8.4.0-beta5, but is required as of PHP 8.4.0-RC1. * This `$escape` issue needs to be addressed directly in the `typo3/testing-framework` and thus needs a raised dependency directly. * PHP 8.4.0.0-RC1 also deprecated the `E_STRICT` constant, which needs to be addressed directly. With [1] the `$escape` parameter for the following method calls * `str_getcsv()` * `fputcsv()` * `fgetcsv()` must be provided as a 1 character long value. Omitting and using the default value will emit a PHP deprecation [2] warning as of PHP 8.4.0-RC1, for example: str_getcsv(): the $escape parameter must be provided as its default value will change To mitigate this, PHP recommends following: It must be passed explicitly either positionally or via named arguments. This change adjusts all occurences (function calls) and ensures that the `$escape` parameter is explicitly provided via specifying all parameters up to that position and not using a `named arguments` approach for easier backporting. The TYPO3 testing framework also needs to be aligned to mitigate `fgetcsv()` issues, and is raised in the same step - otherwise none of these changes would get a green CI pipeline run. The following testing-framework updates are adjusted: * main: simple update `main` pointer * 12.4: Raise to tag `8.2.2` * 11.5: Raise to tag `6.16.10` Used command(s): > composer update "typo3/testing-framework" Additionally, PHP 8.4.0-RC1 deprecated the `E_STRICT` constant, which now emits a E_DEPRECATED which leads to failed CI tests. This change introduces a core internal constant with the same integer value, and replaces usages with this constant to mitigate the E_DEPRECATION. This constant can then be dropped with TYPO3 v14, preventing a breaking change at this time. [3][4] [1] php/php-src#15569 [2] https://github.com/php/php-src/blob/ebee8df27ed/UPGRADING#L617-L622 [3] https://github.com/php/php-src/blob/ebee8df27edf7/UPGRADING#L47-L49 [4] php/php-src#13053 Resolves: #105155 Releases: main, 12.4, 11.5 Change-Id: Ie8b7d46eeb75ba6e32c0e8f6e7e947775083cc15 Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/86371 Tested-by: Garvin Hicking <[email protected]> Reviewed-by: Stefan Bürk <[email protected]> Tested-by: Anja Leichsenring <[email protected]> Reviewed-by: Anja Leichsenring <[email protected]> Tested-by: core-ci <[email protected]> Tested-by: Stefan Bürk <[email protected]> Reviewed-by: Garvin Hicking <[email protected]>

Note that this change addresses three different issues, which need to be done in one step, otherwise none of isolated patches would get a +1 CI verification: * Alignment for changed `$escape` parameter handling of CSV related methods, which must be avoided with PHP versions below 8.4.0-beta5, but is required as of PHP 8.4.0-RC1. * This `$escape` issue needs to be addressed directly in the `typo3/testing-framework` and thus needs a raised dependency directly. * PHP 8.4.0.0-RC1 also deprecated the `E_STRICT` constant, which needs to be addressed directly. With [1] the `$escape` parameter for the following method calls * `str_getcsv()` * `fputcsv()` * `fgetcsv()` must be provided as a 1 character long value. Omitting and using the default value will emit a PHP deprecation [2] warning as of PHP 8.4.0-RC1, for example: str_getcsv(): the $escape parameter must be provided as its default value will change To mitigate this, PHP recommends following: It must be passed explicitly either positionally or via named arguments. This change adjusts all occurences (function calls) and ensures that the `$escape` parameter is explicitly provided via specifying all parameters up to that position and not using a `named arguments` approach for easier backporting. The TYPO3 testing framework also needs to be aligned to mitigate `fgetcsv()` issues, and is raised in the same step - otherwise none of these changes would get a green CI pipeline run. The following testing-framework updates are adjusted: * main: simple update `main` pointer * 12.4: Raise to tag `8.2.2` * 11.5: Raise to tag `6.16.10` Used command(s): > composer require --dev "typo3/testing-framework":"^6.16.10" Additionally, PHP 8.4.0-RC1 deprecated the `E_STRICT` constant, which now emits a E_DEPRECATED which leads to failed CI tests. This change introduces a core internal constant with the same integer value, and replaces usages with this constant to mitigate the E_DEPRECATION. This constant can then be dropped with TYPO3 v14, preventing a breaking change at this time. [3][4] [1] php/php-src#15569 [2] https://github.com/php/php-src/blob/ebee8df27ed/UPGRADING#L617-L622 [3] https://github.com/php/php-src/blob/ebee8df27edf7/UPGRADING#L47-L49 [4] php/php-src#13053 Resolves: #105155 Releases: main, 12.4, 11.5 Change-Id: Ie8b7d46eeb75ba6e32c0e8f6e7e947775083cc15 Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/86377 Tested-by: Stefan Bürk <[email protected]> Reviewed-by: Garvin Hicking <[email protected]> Tested-by: core-ci <[email protected]> Reviewed-by: Stefan Bürk <[email protected]> Tested-by: Garvin Hicking <[email protected]>

Note that this change addresses three different issues, which need to be done in one step, otherwise none of isolated patches would get a +1 CI verification: * Alignment for changed `$escape` parameter handling of CSV related methods, which must be avoided with PHP versions below 8.4.0-beta5, but is required as of PHP 8.4.0-RC1. * This `$escape` issue needs to be addressed directly in the `typo3/testing-framework` and thus needs a raised dependency directly. * PHP 8.4.0.0-RC1 also deprecated the `E_STRICT` constant, which needs to be addressed directly. With [1] the `$escape` parameter for the following method calls * `str_getcsv()` * `fputcsv()` * `fgetcsv()` must be provided as a 1 character long value. Omitting and using the default value will emit a PHP deprecation [2] warning as of PHP 8.4.0-RC1, for example: str_getcsv(): the $escape parameter must be provided as its default value will change To mitigate this, PHP recommends following: It must be passed explicitly either positionally or via named arguments. This change adjusts all occurences (function calls) and ensures that the `$escape` parameter is explicitly provided via specifying all parameters up to that position and not using a `named arguments` approach for easier backporting. The TYPO3 testing framework also needs to be aligned to mitigate `fgetcsv()` issues, and is raised in the same step - otherwise none of these changes would get a green CI pipeline run. The following testing-framework updates are adjusted: * main: simple update `main` pointer * 12.4: Raise to tag `8.2.2` * 11.5: Raise to tag `6.16.10` Used command(s): > composer update "typo3/testing-framework" Note that the backport has been squashed with the later partly revert using integer values instead of adding a own deprecated constant #105165 for [3][4]. We can not deprecate a constant and use it at the same time. We basically traded a deprecated constant with a new deprecated constant. Therefore this intermediate constant (added without being released yet in #105155) is removed again and replaced by a plain value. It's too likely that this constant is used by 3rd party code (or dependencies like typo3/testing-framework), which then makes it hard to remove this constant again (despite being deprecated). Also defining an own constant – that looks like an official PHP constant – into the global space, has caused immediate confusions which we want to avoid by using a scalar value + comment. The reason (possible 3rd party extensions that may still trigger this error) why this is kept (and not just removed) is also added to all usages now. [1] php/php-src#15569 [2] https://github.com/php/php-src/blob/ebee8df27ed/UPGRADING#L617-L622 [3] https://github.com/php/php-src/blob/ebee8df27edf7/UPGRADING#L47-L49 [4] php/php-src#13053 Resolves: #105155 Resolves: #105165 Releases: main, 13.3, 12.4, 11.5 Change-Id: Ie8b7d46eeb75ba6e32c0e8f6e7e947775083cc15 Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/86456 Tested-by: Christian Kuhn <[email protected]> Tested-by: Oliver Hader <[email protected]> Tested-by: core-ci <[email protected]> Reviewed-by: Oliver Hader <[email protected]> Reviewed-by: Christian Kuhn <[email protected]>

With [1] the 5th parameter `$escape` of `fgetcsv()` must be provided either positional or using named arguments or a E_DEPRECATED will be emitted since `PHP 8.4.0 RC1` [2]. This change provide now all five parameter for `fgetcsv()` calls and thus using the positional approach to allow easier backporting to older TYPO3 version where named arguements are not usable. [1] php/php-src#15569 [2] https://github.com/php/php-src/blob/ebee8df27ed/UPGRADING#L617-L622 Releases: main, 8, 7

Girgias requested a review from bukka as a code owner August 24, 2024 14:24

github-actions bot added Extension: spl Extension: standard labels Aug 24, 2024

Girgias force-pushed the cvs-deprecation-improvements branch from 63eefc5 to 6da9f6e Compare August 27, 2024 10:58

github-actions bot added the Extension: phar label Aug 27, 2024

Girgias force-pushed the cvs-deprecation-improvements branch 2 times, most recently from 3c4bbdf to 1fb6ffb Compare September 13, 2024 13:19

Make CSV deprecation less annoying to deal with

4ffd581

Girgias force-pushed the cvs-deprecation-improvements branch from 1fb6ffb to 4ffd581 Compare September 13, 2024 13:21

Girgias merged commit f756b96 into php:master Sep 13, 2024
10 checks passed

Girgias deleted the cvs-deprecation-improvements branch September 13, 2024 14:07

jrfnl mentioned this pull request Sep 17, 2024

[skip ci] PHP 8.4 NEWS: fix typos in parameter names #15931

Merged

jrfnl mentioned this pull request Sep 17, 2024

Account for changes introduced in PHP 8.4 PHPCompatibility/PHPCompatibility#1731

Open

53 tasks

sbuerk mentioned this pull request Sep 29, 2024

[TASK] Explicitly provide all fgetcsv() arguments TYPO3/testing-framework#622

Merged

sbuerk mentioned this pull request Sep 29, 2024

[TASK] Explicitly provide all fgetcsv() arguments TYPO3/testing-framework#623

Merged

sbuerk mentioned this pull request Sep 29, 2024

[TASK] Explicitly provide all fgetcsv() arguments TYPO3/testing-framework#624

Merged

jrfnl mentioned this pull request Oct 6, 2024

PHP 7.4/8.4 | New PHPCompatibility.ParameterValues.RemovedProprietaryCSVEscaping sniff PHPCompatibility/PHPCompatibility#1787

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make CSV deprecation less annoying to deal with #15569

Make CSV deprecation less annoying to deal with #15569

Girgias commented Aug 24, 2024

cmb69 commented Aug 24, 2024

Girgias commented Aug 24, 2024

cmb69 commented Aug 24, 2024

nyamsprod commented Aug 26, 2024 •

edited

Loading

Girgias commented Aug 26, 2024

nyamsprod commented Aug 26, 2024

Girgias commented Aug 26, 2024

nyamsprod commented Aug 26, 2024

jrfnl commented Sep 17, 2024

nyamsprod commented Sep 17, 2024 •

edited

Loading

cmb69 commented Sep 17, 2024

Girgias commented Sep 18, 2024

jrfnl commented Sep 18, 2024

cmb69 commented Sep 19, 2024

jrfnl commented Sep 19, 2024

Girgias commented Sep 20, 2024

bukka commented Sep 23, 2024

Make CSV deprecation less annoying to deal with #15569

Make CSV deprecation less annoying to deal with #15569

Conversation

Girgias commented Aug 24, 2024

cmb69 commented Aug 24, 2024

Girgias commented Aug 24, 2024

cmb69 commented Aug 24, 2024

nyamsprod commented Aug 26, 2024 • edited Loading

Girgias commented Aug 26, 2024

nyamsprod commented Aug 26, 2024

Girgias commented Aug 26, 2024

nyamsprod commented Aug 26, 2024

jrfnl commented Sep 17, 2024

nyamsprod commented Sep 17, 2024 • edited Loading

cmb69 commented Sep 17, 2024

Girgias commented Sep 18, 2024

jrfnl commented Sep 18, 2024

cmb69 commented Sep 19, 2024

jrfnl commented Sep 19, 2024

Girgias commented Sep 20, 2024

bukka commented Sep 23, 2024

nyamsprod commented Aug 26, 2024 •

edited

Loading

nyamsprod commented Sep 17, 2024 •

edited

Loading