Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix usage of non empty $escape in PHP 8.4 #531

Closed
wants to merge 2 commits into from
Closed

Fix usage of non empty $escape in PHP 8.4 #531

wants to merge 2 commits into from

Conversation

cedric-anne
Copy link
Contributor

Using a non empty string for $escape to is deprecated in PHP 8.4, see php/php-src#15365

As far as I understand, using \\ for $enclosure and `` for $escape will result in the same, according to https://www.php.net/manual/fr/function.fgetcsv.php

@nyamsprod
Copy link
Member

nyamsprod commented Aug 19, 2024

@cedric-anne thanks for the submission but I think it is unfortunate that this RFC seems to have passed. As there is no clear workaround or patches for that. The fix you are proposing CAN/SHOULD be considered as a BC breaking change for the whole package. But also for anyone using the csv related functions/methods of PHP. To put it bluntly, the only way to avoid triggering the deprecation warning one will have be to explicitly set the escape parameter to the empty string basically which is NOT the default in PHP 🤷🏾

Changing the default escape sequence register in my book as being a BC break.

This is one of the rare occurence IMHO were a hard BC between PHP8 -> PHP9 would have been better because there is no clear sensible way of fixing that.

@nyamsprod
Copy link
Member

@cedric-anne the enclosure parameter is unaffected by the deprecation warning so I do not think it should be changed at all.
The PR you are referring to is related to str_getcsv which is never used in the package.

@nyamsprod
Copy link
Member

Screenshot_20240819-200717
☝🏾 That's the real issue

@@ -44,8 +44,8 @@ abstract class AbstractCsv implements ByteSequence
protected ?Bom $input_bom = null;
protected ?Bom $output_bom = null;
protected string $delimiter = ',';
protected string $enclosure = '"';
protected string $escape = '\\';
protected string $enclosure = '\\';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change is not required and not needed.

protected string $enclosure = '"';
protected string $escape = '\\';
protected string $enclosure = '\\';
protected string $escape = '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a BC break.

Copy link
Member

@nyamsprod nyamsprod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the change to the enclosure property. For the escape property I am on the fence about changing a default parameter as this introduce a BC break.

@nyamsprod
Copy link
Member

@cedric-anne I will close this PR and redirect you to the issue I just opened which summarizes the problem and what I see for now as potential solutions.
#532

@nyamsprod nyamsprod closed this Aug 20, 2024
@cedric-anne cedric-anne deleted the fix/proprietary-escape branch August 20, 2024 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants