Skip to content

Conversation

oziriemeka
Copy link

Fixes SimpleXMLElement::xpath(): Undefined namespace prefix when reading VML comments/shapes by registering x: (Excel) and o: (Office) namespaces alongside v:.

Includes:

  • tests/PhpSpreadsheetTests/Reader/Xlsx/Issue4505Test.php
  • tests/data/Reader/XLSX/issue.4505.xlsx

Refs: #4505

…ice#4505

Fixes SimpleXMLElement::xpath(): Undefined namespace prefix when reading VML comments/shapes by registering x: (Excel) and o: (Office) namespaces alongside v:.

Includes:
- tests/PhpSpreadsheetTests/Reader/Xlsx/Issue4505Test.php
- tests/data/Reader/XLSX/issue.4505.xlsx

Refs: PHPOffice#4505
@oleibman
Copy link
Collaborator

oleibman commented Oct 3, 2025

Thank you, I will study this over the weekend. Just to put your mind at ease, do not worry about the coverage failure; coveralls has been having a lot of troubles lately.

@oleibman
Copy link
Collaborator

oleibman commented Oct 8, 2025

I am sorry, your test is not adequate. In particular, your file issue.4505.xlsx is corrupt - Excel will not open it without making changes (I am on Excel 365 on Windows 11 if it matters). Please provide a good example of a file where it is usable without changes in Excel, and where loading it in PhpSpreadsheet without your change fails, but loading it with your change succeeds.

It is important to find a spreadsheet which meets those criteria because line 1221 of your Reader/Xlsx module:

$clientData = $shape->xpath('.//x:ClientData');

is covered in 37 different unit tests, and doesn't fail in any of them. This is presumably a line which might have needed your change, but apparently it doesn't. Same for lines 1221, 1257, and 1267. Line 1376 is exercised a lot less, but it is still exercised (3 times), and it uses the v namespace, not the 'o' or 'x' namespace which are the origin of this issue.

And, looking back at the original issue, it seems that this is a case of unexpected namespacing by a 3rd party application generating the failing spreadsheet. I might be able to work with that. In the meantime, I am converting this PR to draft status.

@oleibman oleibman marked this pull request as draft October 8, 2025 19:06
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