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

Crash while trying setting a cell the value "123456\n" #1476

Closed
giannign1 opened this issue May 18, 2020 · 11 comments · Fixed by #1481
Closed

Crash while trying setting a cell the value "123456\n" #1476

giannign1 opened this issue May 18, 2020 · 11 comments · Fixed by #1481

Comments

@giannign1
Copy link
Contributor

giannign1 commented May 18, 2020

This is:

- [X] a bug report
- [ ] a feature request
- [ ] **not** a usage question (ask them on https://stackoverflow.com/questions/tagged/phpspreadsheet or https://gitter.im/PHPOffice/PhpSpreadsheet)

What is the expected behavior?

$worksheet->setCellValueByColumnAndRow(1,1,"123456\n");
Should add the value "123456\n" (or better the number 123456) to the cell at indexes 1,1

What is the current behavior?

the DefaultValueBinder::dataTypeForValue matches (with the preg_match case) the value as a DataType::TYPE_NUMERIC and then the Cell->setValueExplicit crashes during the check if (is_string($pValue) && !is_numeric($pValue))

What are the steps to reproduce?

Try to set to a cell a value with the \n at the end, for example:
$worksheet = //Worksheet instance
$worksheet->setCellValueByColumnAndRow(1,1,"123456\n");

Please provide a Minimal, Complete, and Verifiable example of code that exhibits the issue without relying on an external Excel file or a web server:

<?php

require __DIR__ . '/vendor/autoload.php';

// Create new Spreadsheet object
$spreadsheet = new \PhpOffice\PhpSpreadsheet\Spreadsheet();

// add code that show the issue here...
$spreadsheet->getActiveSheet()->setCellValueByColumnAndRow(1,1,"123456\n");


### Which versions of PhpSpreadsheet and PHP are affected?
v1.12.0
@Vagir-dev
Copy link
Contributor

Hi!
May I ask: why do you use "\n"?

@giannign1
Copy link
Contributor Author

giannign1 commented May 19, 2020

its a value setted custom by our users
i agree that maybe shouldn't be there.... but the lib shouldn't crash anyway, should it?

@Vagir-dev
Copy link
Contributor

You right, it shouldn't.
I asked just to figure out how to help you...

@giannign1
Copy link
Contributor Author

i cited two internal method
one (that who guess the type by the value), using theregex identify it as a number

the other one, who tries to set the value using an explicit type, doesn't recognize it as a number

IMHO the correct one is the second, so MAYBE you only need to check that regex

my two cents

(in my code i solved with a $value = trim($value, '\n') ... but this is only a case specific workaround 😅)

@Vagir-dev
Copy link
Contributor

Vagir-dev commented May 19, 2020

@giannign1, try this patch:

Index: src/PhpSpreadsheet/Cell/DefaultValueBinder.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/PhpSpreadsheet/Cell/DefaultValueBinder.php	(revision 24be4824109f3d647c3d959075129e975530b072)
+++ src/PhpSpreadsheet/Cell/DefaultValueBinder.php	(date 1589910330697)
@@ -65,6 +65,8 @@
                 return DataType::TYPE_STRING;
             } elseif ((strpos($pValue, '.') === false) && ($pValue > PHP_INT_MAX)) {
                 return DataType::TYPE_STRING;
+            } elseif (!is_numeric($pValue)) {
+                return DataType::TYPE_STRING;
             }
 
             return DataType::TYPE_NUMERIC;

Does it solve the problem?

@Vagir-dev
Copy link
Contributor

Vagir-dev commented May 19, 2020

With this patch the result after run your code is:

image

A1 now contains string "123456" with the line break after it.

@giannign1
Copy link
Contributor Author

Great, thank you!!

@giannign1
Copy link
Contributor Author

when will the fix be online? :)

@Vagir-dev
Copy link
Contributor

@giannign1, I don't know. I'm not a member of the PhpSpreadsheet development team. :)

@giannign1
Copy link
Contributor Author

i was thinking of yes lol
So i should submit a PR with your commit?
Or will you do that?

@Vagir-dev
Copy link
Contributor

Vagir-dev commented May 20, 2020

I don't mind at all, if you'll do it. :)

giannign1 added a commit to giannign1/PhpSpreadsheet that referenced this issue May 20, 2020
giannign1 added a commit to giannign1/PhpSpreadsheet that referenced this issue May 20, 2020
giannign1 added a commit to giannign1/PhpSpreadsheet that referenced this issue May 20, 2020
giannign1 added a commit to giannign1/PhpSpreadsheet that referenced this issue May 23, 2020
MarkBaker pushed a commit that referenced this issue May 23, 2020
… line (#1481)

* fix: issue #1476 crash with numeric string value terminating with new line
* test: provided tests for issue #1476
PowerKiKi added a commit that referenced this issue May 31, 2020
### Added

- Support writing to streams in all writers [#1292](#1292)
- Support CSV files with data wrapping a lot of lines [#1468](#1468)
- Support protection of worksheet by a specific hash algorithm [#1485](#1485)

### Fixed

- Fix Chart samples by updating chart parameter from 0 to DataSeries::EMPTY_AS_GAP [#1448](#1448)
- Fix return type in docblock for the Cells::get() [#1398](#1398)
- Fix RATE, PRICE, XIRR, and XNPV Functions [#1456](#1456)
- Save Excel 2010+ functions properly in XLSX [#1461](#1461)
- Several improvements in HTML writer [#1464](#1464)
- Fix incorrect behaviour when saving XLSX file with drawings [#1462](#1462),
- Fix Crash while trying setting a cell the value "123456\n" [#1476](#1481)
- Improved DATEDIF() function and reduced errors for Y and YM units [#1466](#1466)
- Stricter typing for mergeCells [#1494](#1494)

### Changed

- Drop support for PHP 7.1, according to https://phpspreadsheet.readthedocs.io/en/latest/#php-version-support
- Drop partial migration tool in favor of complete migration via RectorPHP [#1445](#1445)
- Limit composer package to `src/` [#1424](#1424)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants