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 to export csv file with numbers in an element #2048

Closed
wants to merge 1 commit into from
Closed

Fix to export csv file with numbers in an element #2048

wants to merge 1 commit into from

Conversation

joern19
Copy link

@joern19 joern19 commented Apr 30, 2021

When trying to export an Spreadsheet with an field only containing numbers, the following error occurs:
PHP Fatal error: Uncaught TypeError: str_replace(): Argument #3 ($subject) must be of type array|string, int given
Fixed by wrapping the element with strval

This is:

- [ ] a bugfix
- [ ] a new feature

Checklist:

Why this change is needed?

When trying to export an Spreadsheet with an field only containing numbers, the following error occurs:
PHP Fatal error:  Uncaught TypeError: str_replace(): Argument #3 ($subject) must be of type array|string, int given
Fixed by wrapping the element with strval
@oleibman
Copy link
Collaborator

I am not able to duplicate your error with the following code:

<?php
declare(strict_types=1);
require __DIR__ . '/PhpSpreadsheet' . '/vendor/autoload.php';
$obj = new PhpOffice\PhpSpreadsheet\Spreadsheet();
$sheet0 = $obj->getActiveSheet();
$sheet0->setCellValue('A1', 2);
$sheet0->setCellValue('A2', 3);
$sheet0->setCellValue('B1', 4);
$sheet0->setCellValue('B2', 5);
$out = 'strict4.csv';
$writer = new \PhpOffice\PhpSpreadsheet\Writer\Csv($obj);
$writer->setEnclosureRequired(true);
$writer->save($out);
echo "$out has been saved\n";

I stuck a var_dump at the code where you saw a failure, and verified that $element was an int all 4 times. I tried this with several versions of PHP, including 8.0.0. Can you share your source code and your PHP version to trigger this problem? Also, which version of PhpSpreadsheet, and is your strval change the only change you made to Writer/Csv?

@joern19
Copy link
Author

joern19 commented Apr 30, 2021

Now I feel stupid. I have added a small feature to the csvwriter and also added declare(strict_types=1); because we add that to all PHP classes. Sorry for the inconvenience caused

@joern19 joern19 closed this Apr 30, 2021
oleibman added a commit to oleibman/PhpSpreadsheet that referenced this pull request Jul 23, 2021
PhpSpreadsheet writes boolean values to a Csv as null-string/1, and treats input values of 'true' and 'false' as if they were strings. On the other hand, Excel writes boolean values to a Csv as TRUE/FALSE, and case-insensitively treats a matching string as boolean on read. This PR changes PhpSpreadsheet to match Excel.

A side-effect of this change is that it fixes behavior incorrectly reported as a bug in PR PHPOffice#2048. That issue was closed, correctly, as user error. The user had altered Csv Writer, including adding ```declare(strict_types=1);```; that declaration was the cause of the error. The "offending" statements, calls to strpbrk and str_replace, will now work correctly whether or not strict_types is in use.

And, just as I was getting ready to push this, the dailies for PHP 8.1 introduced a change deprecating auto_detect_line_endings. Csv Reader uses that setting; it allows it to process a Csv with Mac line endings, which happens to be something that Excel can do. As they say in https://wiki.php.net/rfc/deprecations_php_8_1, where the proposal passed without a single dissenting vote, "These newlines were used by “Classic” Mac OS, a system which has been discontinued in 2001, nearly two decades ago. Interoperability with such systems is no longer relevant." I tend to agree, but I don't know that we're ready to pull the plug yet. I don't see an easy way to emulate that functionality. For now, I have silenced the deprecation notices with at signs. I have also added a test case which will fail when support for that setting is pulled; this will give time to consider alternatives.
oleibman added a commit that referenced this pull request Aug 4, 2021
* Csv Handling of Booleans (and an 8.1 Deprecation)

PhpSpreadsheet writes boolean values to a Csv as null-string/1, and treats input values of 'true' and 'false' as if they were strings. On the other hand, Excel writes boolean values to a Csv as TRUE/FALSE, and case-insensitively treats a matching string as boolean on read. This PR changes PhpSpreadsheet to match Excel.

A side-effect of this change is that it fixes behavior incorrectly reported as a bug in PR #2048. That issue was closed, correctly, as user error. The user had altered Csv Writer, including adding ```declare(strict_types=1);```; that declaration was the cause of the error. The "offending" statements, calls to strpbrk and str_replace, will now work correctly whether or not strict_types is in use.

And, just as I was getting ready to push this, the dailies for PHP 8.1 introduced a change deprecating auto_detect_line_endings. Csv Reader uses that setting; it allows it to process a Csv with Mac line endings, which happens to be something that Excel can do. As they say in https://wiki.php.net/rfc/deprecations_php_8_1, where the proposal passed without a single dissenting vote, "These newlines were used by “Classic” Mac OS, a system which has been discontinued in 2001, nearly two decades ago. Interoperability with such systems is no longer relevant." I tend to agree, but I don't know that we're ready to pull the plug yet. I don't see an easy way to emulate that functionality. For now, I have silenced the deprecation notices with at signs. I have also added a test case which will fail when support for that setting is pulled; this will give time to consider alternatives.

* Scrutinizer: Handling ini_set

This could be interesting. It doesn't like not handling an error condition for ini_set. Let's see if this satisfies it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants