Skip to content

Commit b82756d

Browse files
oleibmanoleibman
committed
Don't Use htmlspecialchars When Formatting Xml
Fix #4537. PhpSpreadsheet currently changes apostrophes in text values to `'`. This is perfectly valid Xml. Issue was opened because R does not handle this correctly; this is unquestionably a bug on R's part. So I was not inclined to do anything about it. However ... User suggested a change to how `htmlspecialchars` was called. Investigating the use of that routine in PhpSpreadsheet, I found that there was some double escaping going on for cells whose type was set to `TYPE_INLINE` - `htmlspecialchars` escaped the string correctly, but it was later written as Xml using a method which escaped the data a second time. So, a real bug in PhpSpreadsheet after all. There was one call to `htmlspecialchars` in `Shared\XmlWriter`. I replaced `writeRaw(htmlspecialchars(...))` with `text(...)`. And one call in `Writer\Xlsx\Worksheet`, the source of the double escaping bug above; the call to `htmlspecialchars` can just be eliminated there. Making those changes, the only remaining calls to `htmlspecialchars` are in `Writer\Html`, where they belong. As a bonus, apostrophes now wind up unescaped, so R will be satisfied (even though they should fix their bug).
1 parent 747ccd1 commit b82756d

File tree

4 files changed

+142
-6
lines changed

4 files changed

+142
-6
lines changed

‎src/PhpSpreadsheet/Shared/XMLWriter.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,6 @@ public function writeRawData($rawTextData): bool
9191
$rawTextData = implode("\n", $rawTextData);
9292
}
9393

94-
return $this->writeRaw(htmlspecialchars($rawTextData ?? ''));
94+
return $this->text($rawTextData ?? '');
9595
}
9696
}

‎src/PhpSpreadsheet/Writer/Xlsx/Worksheet.php‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010
use PhpOffice\PhpSpreadsheet\Cell\DataType;
1111
use PhpOffice\PhpSpreadsheet\Reader\Xlsx\Namespaces;
1212
use PhpOffice\PhpSpreadsheet\RichText\RichText;
13-
use PhpOffice\PhpSpreadsheet\Settings;
1413
use PhpOffice\PhpSpreadsheet\Shared\StringHelper;
1514
use PhpOffice\PhpSpreadsheet\Shared\XMLWriter;
1615
use PhpOffice\PhpSpreadsheet\Style\Conditional;
@@ -1440,10 +1439,7 @@ private function writeCellInlineStr(XMLWriter $objWriter, string $mappedType, Ri
14401439
$objWriter->writeElement(
14411440
't',
14421441
StringHelper::controlCharacterPHP2OOXML(
1443-
htmlspecialchars(
1444-
$cellValue,
1445-
Settings::htmlEntityFlags()
1446-
)
1442+
$cellValue
14471443
)
14481444
);
14491445
$objWriter->endElement();

‎tests/PhpSpreadsheetTests/Writer/Ods/Issue4537Test.php‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace PhpOffice\PhpSpreadsheetTests\Writer\Ods;
6+
7+
use PhpOffice\PhpSpreadsheet\Cell\DataType;
8+
use PhpOffice\PhpSpreadsheet\Reader\Ods as OdsReader;
9+
use PhpOffice\PhpSpreadsheet\RichText\RichText;
10+
use PhpOffice\PhpSpreadsheet\RichText\TextElement;
11+
use PhpOffice\PhpSpreadsheet\Shared\File;
12+
use PhpOffice\PhpSpreadsheet\Spreadsheet;
13+
use PhpOffice\PhpSpreadsheet\Writer\Ods as OdsWriter;
14+
use PHPUnit\Framework\TestCase;
15+
16+
class Issue4537Test extends TestCase
17+
{
18+
private string $outputFilename = '';
19+
20+
protected function tearDown(): void
21+
{
22+
if ($this->outputFilename !== '') {
23+
unlink($this->outputFilename);
24+
$this->outputFilename = '';
25+
}
26+
}
27+
28+
public function testBackgroundImage(): void
29+
{
30+
$this->outputFilename = File::temporaryFilename();
31+
$testString = "\"He\": '<?>'";
32+
$spreadsheet = new Spreadsheet();
33+
$sheet = $spreadsheet->getActiveSheet();
34+
$sheet->getCell('A1')->setValueExplicit($testString, DataType::TYPE_INLINE);
35+
$sheet->getCell('A2')->setValue($testString);
36+
$richText = new RichText();
37+
$richText->addText(new TextElement($testString));
38+
$sheet->getCell('A3')->setValue($richText);
39+
$writer = new OdsWriter($spreadsheet);
40+
$writer->save($this->outputFilename);
41+
$spreadsheet->disconnectWorksheets();
42+
43+
$reader = new OdsReader();
44+
$reloadedSpreadsheet = $reader->load($this->outputFilename);
45+
$rsheet = $reloadedSpreadsheet->getActiveSheet();
46+
self::assertSame($testString, $rsheet->getCell('A1')->getValueString());
47+
self::assertSame($testString, $rsheet->getCell('A2')->getValueString());
48+
self::assertSame($testString, $rsheet->getCell('A3')->getValueString());
49+
$reloadedSpreadsheet->disconnectWorksheets();
50+
51+
$file = 'zip://';
52+
$file .= $this->outputFilename;
53+
$file .= '#content.xml';
54+
$data = file_get_contents($file);
55+
// expected does not escape apostrophes
56+
$expected = '<text:p>&quot;He&quot;: \'&lt;?&gt;\'</text:p>';
57+
if ($data === false) {
58+
self::fail('Unable to read content file');
59+
} else {
60+
$count = substr_count($data, $expected);
61+
self::assertSame(3, $count);
62+
}
63+
}
64+
}

‎tests/PhpSpreadsheetTests/Writer/Xlsx/Issue4537Test.php‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace PhpOffice\PhpSpreadsheetTests\Writer\Xlsx;
6+
7+
use PhpOffice\PhpSpreadsheet\Cell\DataType;
8+
use PhpOffice\PhpSpreadsheet\Reader\Xlsx as XlsxReader;
9+
use PhpOffice\PhpSpreadsheet\RichText\RichText;
10+
use PhpOffice\PhpSpreadsheet\RichText\TextElement;
11+
use PhpOffice\PhpSpreadsheet\Shared\File;
12+
use PhpOffice\PhpSpreadsheet\Spreadsheet;
13+
use PhpOffice\PhpSpreadsheet\Writer\Xlsx as XlsxWriter;
14+
use PHPUnit\Framework\TestCase;
15+
16+
class Issue4537Test extends TestCase
17+
{
18+
private string $outputFilename = '';
19+
20+
protected function tearDown(): void
21+
{
22+
if ($this->outputFilename !== '') {
23+
unlink($this->outputFilename);
24+
$this->outputFilename = '';
25+
}
26+
}
27+
28+
public function testBackgroundImage(): void
29+
{
30+
$this->outputFilename = File::temporaryFilename();
31+
$testString = "\"He\": '<?>'";
32+
$spreadsheet = new Spreadsheet();
33+
$sheet = $spreadsheet->getActiveSheet();
34+
$sheet->getCell('A1')->setValueExplicit($testString, DataType::TYPE_INLINE);
35+
$sheet->getCell('A2')->setValue($testString);
36+
$richText = new RichText();
37+
$richText->addText(new TextElement($testString));
38+
$sheet->getCell('A3')->setValue($richText);
39+
$writer = new XlsxWriter($spreadsheet);
40+
$writer->save($this->outputFilename);
41+
$spreadsheet->disconnectWorksheets();
42+
43+
$reader = new XlsxReader();
44+
$reloadedSpreadsheet = $reader->load($this->outputFilename);
45+
$rsheet = $reloadedSpreadsheet->getActiveSheet();
46+
self::assertSame($testString, $rsheet->getCell('A1')->getValueString());
47+
self::assertSame($testString, $rsheet->getCell('A2')->getValueString());
48+
self::assertSame($testString, $rsheet->getCell('A3')->getValueString());
49+
$reloadedSpreadsheet->disconnectWorksheets();
50+
51+
$file = 'zip://';
52+
$file .= $this->outputFilename;
53+
$file .= '#xl/worksheets/sheet1.xml';
54+
$data = file_get_contents($file);
55+
// expected, and expected1/2 below, do not escape apostrophes
56+
$expected = 't="inlineStr"><is><t>&quot;He&quot;: \'&lt;?&gt;\'</t></is>';
57+
if ($data === false) {
58+
self::fail('Unable to read worksheets file');
59+
} else {
60+
self::assertStringContainsString($expected, $data, 'inline string');
61+
}
62+
63+
$file = 'zip://';
64+
$file .= $this->outputFilename;
65+
$file .= '#xl/sharedStrings.xml';
66+
$data = file_get_contents($file);
67+
$expected1 = '<t>&quot;He&quot;: \'&lt;?&gt;\'</t>';
68+
$expected2 = '<t xml:space="preserve">&quot;He&quot;: \'&lt;?&gt;\'</t>';
69+
if ($data === false) {
70+
self::fail('Unable to read sharedStrings file');
71+
} else {
72+
self::assertStringContainsString($expected1, $data, 'string');
73+
self::assertStringContainsString($expected2, $data, 'rich text');
74+
}
75+
}
76+
}

0 commit comments

Comments
 (0)