Using htmlspecialchars to fix issue #3145

[zappzerapp]

This is:

- [X] a bugfix
- [ ] a new feature
- [ ] refactoring
- [ ] additional unit tests

Checklist:

• Changes are covered by unit tests
  • Changes are covered by existing unit tests
  • New unit tests have been added
• Code style is respected
• Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
• CHANGELOG.md contains a short summary of the change
• Documentation is updated as necessary

Why this change is needed?

This prevents the SimpleXMLElement error "unterminated entity reference" when HTML entities are present in a worksheet #3145

[oleibman]

I am a bit concerned that the use of htmlspecialchars here might break other things for which we don't yet have unit tests. I need to run some more tests, but the following also fixes your test case and seems safer.

$childNode = $node->addChild('formula1');
if ($childNode !== null) { // null should never happen
    $childNode[0] = (string) $item->formula1->children(Namespaces::DATA_VALIDATIONS2)->f;
}

See https://stackoverflow.com/questions/552957/rationale-behind-simplexmlelements-handling-of-text-values-in-addchild-and-adda
And https://stackoverflow.com/questions/2970575/php-simplexmlelement-set-text

[zappzerapp]

@oleibman have a look at the phpstan action

 Error: SimpleXMLElement does not accept string.
 ------ ------------------------------------------ 
  Line   src/PhpSpreadsheet/Reader/Xlsx.php        
 ------ ------------------------------------------ 
  945    SimpleXMLElement does not accept string.  
 ------ ------------------------------------------ 

[oleibman]

I am not sure I agree with PhpStan's analysis. I will open an issue there. In the meantime, I think it is safe to eliminate the string cast. Please give that a try. I have another variant in mind if that doesn't work.

[zappzerapp]

I think we need your other variant 😄

[oleibman]

I am sorry this is causing such a problem. I will have something ready for you later today.

[oleibman]

Please re-code the erring line as:

$childNode[0] = (string) $item->formula1->children(Namespaces::DATA_VALIDATIONS2)->f; // @phpstan-ignore-line

I have verified that (a) it works, and (b) Phpstan doesn't complain. I apologize for not doing the (b) part for my earlier suggestions. We don't like using the annotations, but sometimes you just can't get around it.

To your test, please add an assertion that the formula is as expected:

self::assertSame(expected formula, $sheet->getCell('B2')->getDataValidation()->getFormula1());

Also please add at the very end of your test:

$spreadsheet->disconnectWorksheets();

[zappzerapp]

Now everything looks good 🙂

[oleibman]

Thank you for your contribution, and your patience.