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

PHP 8.1 mb_substr deprecation message when setting cell value using setValueExplicit with a null string #2868

Closed
yblatti opened this issue Jun 2, 2022 · 11 comments

Comments

@yblatti
Copy link

yblatti commented Jun 2, 2022

This is:

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

Happens when I use $cell->setValueExplicit() with null value and a string(ish) type cell. Using PHP 8.1

What is the expected behavior?

No deprecation message, same as PHP <= 8.0

What is the current behavior?

Get message :
Deprecated: mb_substr(): Passing null to parameter #1 ($string) of type string is deprecated

What are the steps to reproduce?

Using PHP 8.1, run :

<?php

use PhpOffice\PhpSpreadsheet\Cell\DataType;
use PhpOffice\PhpSpreadsheet\Spreadsheet;
use PhpOffice\PhpSpreadsheet\Worksheet\Worksheet;

require_once('vendor/autoload.php');

$testTypes = [
    'TYPE_STRING2' => DataType::TYPE_STRING2,
    'TYPE_STRING' => DataType::TYPE_STRING,
    'TYPE_INLINE' => DataType::TYPE_INLINE,
];

$spreadsheet = new Spreadsheet();
$sheet = new Worksheet($spreadsheet);

foreach ($testTypes as $testTypeName => $testType) {
    printf("Testing for type : %s ...\n", $testTypeName);
    $cell = $sheet->getCellByColumnAndRow(0, 0)->setValueExplicit(null, $testType);
}

Output :

Testing for type : TYPE_STRING2 ...

Deprecated: mb_substr(): Passing null to parameter #1 ($string) of type string is deprecated in /var/www/html/vendor/phpoffice/phpspreadsheet/src/PhpSpreadsheet/Shared/StringHelper.php on line 481
Testing for type : TYPE_STRING ...

Deprecated: mb_substr(): Passing null to parameter #1 ($string) of type string is deprecated in /var/www/html/vendor/phpoffice/phpspreadsheet/src/PhpSpreadsheet/Shared/StringHelper.php on line 481
Testing for type : TYPE_INLINE ...

Deprecated: mb_substr(): Passing null to parameter #1 ($string) of type string is deprecated in /var/www/html/vendor/phpoffice/phpspreadsheet/src/PhpSpreadsheet/Shared/StringHelper.php on line 481

Does an issue affect all spreadsheet file formats?

Yes (format independent)

Which versions of PhpSpreadsheet and PHP are affected?

1.23.0 (previous versions not tested)

yblatti pushed a commit to yblatti/PhpSpreadsheet that referenced this issue Jun 2, 2022
…ge (PHP 8.1) in StringHelper::substring()
yblatti pushed a commit to yblatti/PhpSpreadsheet that referenced this issue Jun 2, 2022
yblatti pushed a commit to yblatti/PhpSpreadsheet that referenced this issue Jun 2, 2022
…ge (PHP 8.1) in StringHelper::substring()
yblatti pushed a commit to yblatti/PhpSpreadsheet that referenced this issue Jun 2, 2022
…ge (PHP 8.1) in StringHelper::substring()
@MarkBaker
Copy link
Member

It raises the question of whether this should be resolved in PhpSpreadsheet or in user code. Should you be allowed to pass a non-string value (such as a null) to StringHelper::substring()?

@yblatti
Copy link
Author

yblatti commented Jun 7, 2022

It raises the question of whether this should be resolved in PhpSpreadsheet or in user code. Should you be allowed to pass a non-string value (such as a null) to StringHelper::substring()?

Good question.

In this case, I used setValueExplicit(), which accepts mixed for $value. And mixed contains null.

I've now stumble upon this chapter of the documentation, which states :

Internally, PhpSpreadsheet uses a default \PhpOffice\PhpSpreadsheet\Cell\IValueBinder implementation (\PhpOffice\PhpSpreadsheet\Cell\DefaultValueBinder) to determine data types of entered data using a cell's setValue() method (the setValueExplicit() method bypasses this check).

Looking at DefaultValueBinder::dataTypeForValue(), I guess that all php null should bind to DataType::TYPE_NULL.

So clearly, I have to fix this in my code ! my bad

@yblatti yblatti closed this as completed Jun 7, 2022
@FlameStorm
Copy link
Contributor

Hi! I discovered this deprecation message appears just under the hood of phpoffice/phpspreadshit with no any case to avoid it in our local code. So this problem and that PR is actual. Vote up for fix.

@MarkBaker
Copy link
Member

If I do add validation checks here to ensure that the data value that you set matches the datatype that you have said it should be, then it will impact performance, and I will throw exceptions... IMO the end-user developer should be responsible for ensuring that the value matches the datatype when they use setValueExplicit(), because they're telling PhpSpreadsheet that they're accepting the responsibility for matching value and type; if they can't do that, then they should be calling setValue() instead.

@FlameStorm
Copy link
Contributor

Sorry, but in my case function setValueExplicit was called inside the library during loading an XLSX file,
image

@FlameStorm
Copy link
Contributor

Passing null to parameter #1 ($string) of type string is deprecated

╵ /var/www/vendor/phpoffice/phpspreadsheet/src/PhpSpreadsheet/Shared/StringHelper.php:481
╵ /var/www/vendor/phpoffice/phpspreadsheet/src/PhpSpreadsheet/Cell/DataType.php:61
╵ /var/www/vendor/phpoffice/phpspreadsheet/src/PhpSpreadsheet/Cell/Cell.php:223
╵ /var/www/vendor/phpoffice/phpspreadsheet/src/PhpSpreadsheet/Reader/Xlsx.php:842
╵ /var/www/vendor/phpoffice/phpspreadsheet/src/PhpSpreadsheet/Reader/BaseReader.php:166

@FlameStorm
Copy link
Contributor

So i think it' a good idea to just reanimate PR #2869
Thank you for support and an awesome library!

@FlameStorm
Copy link
Contributor

End-user developer code is just a such two rows
image

@MarkBaker
Copy link
Member

MarkBaker commented Jun 10, 2022

A very strange Xlsx file, certainly one created by MS Excel itself should never have a null value typed as a string... anyway it's done, I've fixed the deprecation notice, without causing any noticeable degradation of performance, and I have updated the documentation.

@yblatti
Copy link
Author

yblatti commented Jun 12, 2022

Thanks for the fix.
But above all: thanks for all your work on these great libraries making our work so easy ❤️

@FlameStorm
Copy link
Contributor

Oh, Mark, really sorry for your feelings! I just can try to understand how you tired "a bit". But, seriously, I can't agree with you - you are really cool, and your open source child is brilliant, I see, I'm firmly sure, and any kind of shit it absolutely not about here. Just forgive some bits of stupidity of MS Excel and other external soft :) Not only some legacy code of interstellar Voyagers, but even "modern" software often do some bullshit, just forgive them :) Thank you !

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.

3 participants