Skip to content

Don't infer cell t attribute from formula source when pre-calc is off#4864

Merged
oleibman merged 1 commit into
PHPOffice:masterfrom
williamdes:fix/no-spurious-t-attribute-when-pre-calc-off
Apr 30, 2026
Merged

Don't infer cell t attribute from formula source when pre-calc is off#4864
oleibman merged 1 commit into
PHPOffice:masterfrom
williamdes:fix/no-spurious-t-attribute-when-pre-calc-off

Conversation

@williamdes
Copy link
Copy Markdown
Contributor

@williamdes williamdes commented Apr 29, 2026

Don't infer cell t attribute from formula source when pre-calc is off

What

When setPreCalculateFormulas(false) is set on the Xlsx writer, every formula cell is written with t="str" — even when the formula evaluates to a number or boolean. This PR removes that spurious type attribute.

Why this matters

Spreadsheet readers use the t attribute on a formula cell as a hint about the result type. Writing t="str" for a =SUMIFS(...) cell tells readers "this formula's result is a string" — which is wrong, and triggers two real failures in the wild:

  1. LibreOffice (versions before 7?) and Gnumeric treat t="str" as authoritative when there is no cached <v>. They either skip recomputation, render the formula text literally, or surface #NAME errors for numeric formulas they would otherwise compute correctly.
  2. Auditors and downstream tooling that introspect the XML get a misleading signal. A cell whose formula returns a number is declared as a string-result cell.

Internally we hit this when generating a TVA report whose Sommaire sheet uses SUMIFS formulas referencing a 20k+ row data sheet. With pre-calc on, PhpSpreadsheet's calc engine iterates whole-column refs to Excel's 1,048,576-row max per condition per formula and the export hangs. Disabling pre-calc unblocks the export but produces a file where every SUMIFS displays as =_xlfn.sumifs(...) returning #NAME because of the t="str" plus the formula prefix combination.

Root cause (src/PhpSpreadsheet/Writer/Xlsx/Worksheet.php, writeCellFormula)

$calculatedValue = $this->getParentWriter()->getPreCalculateFormulas()
    ? $cell->getCalculatedValue()
    : $cellValue;          // ← formula source string when pre-calc is off
// ...
if (is_string($result)) {  // ← always true when $result derives from $cellValue
    // ...
    $objWriter->writeAttribute('t', 'str');
}

$cellValue is the formula source (e.g. "=SUMIFS(...)"). It is always a string, so the is_string branch always fires when pre-calc is off, regardless of what the formula would actually evaluate to.

Fix

Leave $calculatedValue null (and $calculatedValueString empty) when pre-calc is off. The is_string/is_bool type-inference branches no longer fire and no t attribute is written. The <v> element is already correctly suppressed by the existing getPreCalculateFormulas() guard at the writeElementIf site, so no cached value is written either.

Result: cells written without pre-calc now look like

<c r="B2"><f>3+A3</f></c>

instead of

<c r="B2" t="str"><f>3+A3</f></c>

The workbook's <calcPr fullCalcOnLoad="1" forceFullCalc="1"/> is unchanged, so readers correctly recompute on open.

Backwards compatibility

  • Pre-calc on (default): no behavioural change. $calculatedValue still comes from $cell->getCalculatedValue() and the existing type-inference logic runs unchanged.
  • Pre-calc off: cells previously wrote t="str" and (until very recently) <v>0</v>. The <v>0</v> was already removed in master; this PR removes the spurious t="str". Readers that handled the old output continue to work, and readers that didn't (per the issues above) now do.

Tests

  • Updates tests/PhpSpreadsheetTests/Writer/PreCalcTest.php line 111 to expect <c r="B2"><f>3+A3</f></c> (no t="str") when $preCalc === false. The Xls / Ods / Html / Csv branches are unaffected.
  • Other tests asserting t="str" on formula cells (Issue2082Test, XlfnFunctionsTest, …) are all in pre-calc=on scenarios with <v>...</v> cached values, and continue to pass unchanged.
  • Verified locally: full tests/PhpSpreadsheetTests/Writer/Xlsx/ suite passes (223 tests), full Functional/ + Calculation/ suites pass (16,265 tests, 0 failures).

Related

  • The companion concern of _xlfn. prefix being added to formula cells when pre-calc is off (which causes #NAME in pre-7 LibreOffice / Gnumeric) is intentionally not addressed in this PR. The _xlfn. prefix is correct OOXML and would touch many more tests; it can be a separate discussion if the project wants to make that conditional too.

More

Commit 406988e — "SetCalculatedValue Avoid Casting String to Numeric (#3685)" by oleibman, 2023-08-26.

The commit message explicitly acknowledges our exact bug:

"Reading a spreadsheet which has been created with preCalculateFormulas false will, or at least should, result in a null oldCalculatedValue. … Xlsx was actually storing 0 in this situation; that wasn't
precisely a problem, but it wasn't necessary and seems misleading. Xlsx Writer is changed to no longer set a value in this situation."

…s off

writeCellFormula() falls back to using $cellValue (the formula source) as
$calculatedValue when getPreCalculateFormulas() is false. The source is
always a string, so the is_string($result) branch always fires, every
formula cell ends up with t="str" — even those whose formula evaluates
to a number or boolean.

Some readers (notably older LibreOffice and Gnumeric) treat t="str" as
a hint that the cell is a string, and either skip recomputation or
present a #NAME error for numeric formulas they would otherwise compute
correctly.

Fix: leave $calculatedValue null and $calculatedValueString empty when
pre-calc is off. The type-inference branches no longer fire, so no t
attribute is written. The surrounding writeElementIf already guards <v>
emission on getPreCalculateFormulas(), so no cached value is written
either — readers see "formula with no cached value, no type hint",
respect the workbook's fullCalcOnLoad="1", and recompute on open.

Updates the existing PreCalcTest assertion to match: <c r="B2"><f>3+A3</f></c>
instead of <c r="B2" t="str"><f>3+A3</f></c>.
@williamdes williamdes marked this pull request as ready for review April 29, 2026 17:40
@williamdes
Copy link
Copy Markdown
Contributor Author

Hi,

I made this PR using @claude Code for one of my clients. I have read everything twice. We did patch the composer library to test if it worked fine.

Please let me know what you think of this PR, I can jump into manual edits.

@oleibman oleibman added this pull request to the merge queue Apr 30, 2026
@oleibman
Copy link
Copy Markdown
Collaborator

Thank you for your contribution. Sorry I didn't notice the second problem when I fixed the first; glad you were able to notice and do something about it.

Merged via the queue into PHPOffice:master with commit d5cdc29 Apr 30, 2026
15 checks passed
@williamdes williamdes deleted the fix/no-spurious-t-attribute-when-pre-calc-off branch April 30, 2026 06:35
@williamdes
Copy link
Copy Markdown
Contributor Author

williamdes commented Apr 30, 2026

Thank you for your contribution. Sorry I didn't notice the second problem when I fixed the first; glad you were able to notice and do something about it.

Thank you!
If you still release 1.x versions would you pick the two patches?
Because the vendor we use is still on v1: https://github.com/SpartnerNL/Laravel-Excel/blob/1854739267d81d38eae7d8c623caf523f30f256b/composer.json#L25

Something about SpartnerNL/Laravel-Excel#4345

@oleibman
Copy link
Copy Markdown
Collaborator

The release 1 branch now receives security patches only. Sorry,

@oleibman
Copy link
Copy Markdown
Collaborator

You might want to add a comment at SpartnerNL/Laravel-Excel#4345, which I believe you referenced above, and which contains instructions for a workaround which multiple others are satisfied with, e.g. SpartnerNL/Laravel-Excel#4345 (comment)

@williamdes
Copy link
Copy Markdown
Contributor Author

You might want to add a comment at SpartnerNL/Laravel-Excel#4345, which I believe you referenced above, and which contains instructions for a workaround which multiple others are satisfied with, e.g. SpartnerNL/Laravel-Excel#4345 (comment)

Yeah okay haha I will keep my vendor patch, it works fine and I will have no trouble with it

Thanks anyway

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