Please consider changing DefaultValueBinder to always bind strings as TYPE_STRING

[jdufresne]

This is a feature request.

When writing a document, please consider changing the Cell::setValue() behavior to always treat strings as strings. Right now, strings that are formatted as a number are coerced and treated like a number. IMO, this causes confusing behavior. If I want to store a number, I would pass a number. But as I pass a string, I expect PhpSpreadsheet to store a string. There are many use cases where strings that look like numbers should be treated as strings: telephone numbers, employee ID numbers, purchase order numbers, etc. I've run into this enough times, that I now use wrappers around PhpSpreadsheet to workaround this. However, I think this is fundamental enough of a concern that PhpSpreadsheet should consider changing the behavior.

I realize this would be a backwards incompatible change. Some people may be relying on this implicit coercion. I'd argue they should be using numerical types for numerical data. If that is too big of a concern, would it be OK to emit a deprecation warning in DefaultValueBinder::dataTypeForValue() to declare that this behavior will change in a future version?

What is the expected behavior?

When creating and writing a document, $cell->setValue('1234') is stored with type TYPE_STRING.

What is the current behavior?

$cell->setValue('1234') is stored with type TYPE_NUMERIC.

What are the steps to reproduce?

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';

$spreadsheet = new \PhpOffice\PhpSpreadsheet\Spreadsheet();
$worksheet = $spreadsheet->getActiveSheet();
$cell = $worksheet->getCell('A1');
$cell->setValue('1234');
echo $cell->getDataType() . "\n";

Which versions of PhpSpreadsheet and PHP are affected?

develop branch

[MarkBaker]

This is why we provide setCellValueExplicit(), and the otion to use the advanced binder or create your own; and why this is fully documented

[jdufresne]

Always using setCellValueExplicit() means I can't use autodetection for stronger types (int, float, DateTime, etc.) which would be unfortunate and limiting.

I agree it is documented, but even so I think the interface could be improved and be less surprising.

[midlan]

I ran into same problem. Extending DefaultValueBinder fixed it for me.

@jdufresne I think you look at the problem only from one side. You are demonstrating it on creating new document manually. But document can be created by reader as well, e.g. CSV. When reading CSV, all cells is read as PHP datatype string, whatever the original type was.

In my case I prefer all CSV cells to be treated as strings, but I can imagine there would be lots of people prefering type autodection.

The right solution to me would be moving the autodection functionality to readers. Reader than would pass the value with right PHP datatype (autodetected) to ValueBinder. In the reader the autodetect functionality could be enabled/disabled.

[PowerKiKi]

This change would probably align with the current trend of making PHP more strict about type. I guess it make sense in that way, and it may be less surprising for some cases.

On the other hand I can see how a lot of less experienced people could get a bit confused if they don't care much about types. Typically fetching a number from DB would give you a string and passing that directly to PhpSpreadsheet would then probably not match expectations.

CSV readers is an excellent argument against this change too. Or at the very least shows that it should be done carefully.

Some sort of compromise could be to do this change and also rename the binder we have today to have more expressive names. DefaultValueBinder could become StrictTypesBinder and AdvancedValueBinder could be something like WeakTypesBinder ?

Whatever we decide this won't land before a 2.0, and it will have to get approval from @MarkBaker.

[jdufresne]

CSV readers is an excellent argument against this change too. Or at the very least shows that it should be done carefully.

Yeah, this use case makes sense. As CSV documents contain no type information, maybe the CSV reader should default to a different binder than writers.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
If this is still an issue for you, please try to help by debugging it further and sharing your results.
Thank you for your contributions.

[oleibman]

StringValueBinder was introduced with PR #901 in Feb. 2019.