fix: do not trigger Undefined array key "x"

[karrakoliko]

This is:

• 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 and a link to the pull request if applicable
• Documentation is updated as necessary

Why this change is needed?

fixes #4668

[oleibman]

Can you add a unit test which will fail without your change but succeed with it?

[karrakoliko]

@oleibman done

[oleibman]

Thank you for adding the test. However ... the code in question is for an Xml file, not Xls. Please rename your file accordingly and move it to tests/data/Reader/Xml, and move your test to tests/PhpSpreadsheetTests/Reader/Xml.

It's a bit of a surprise that this is a problem, because the Xml file does define a namespace 'x'. However, the getNameSpaces function returns the namespaces used in the document, not the namespaces defined in it. It would not surprise me if there are more surprises of this nature, but we needn't delay your change on that account.

[karrakoliko]

@oleibman done

getNameSpaces function returns the namespaces used in the document, not the namespaces defined in it

i can easily beleive that the file itself is semi-valid. i removed all the xml rows from it because they contain highly sensitive data.

probably it would be a better idea to anonymize data instead (John Doe, card number 111111***2222 etc), so it could potentitally be helpful for understanding the context (?).

in this specific case it is just about hardcoded array key without proper check, so i'm not ready to spend time on anonymization.

[karrakoliko]

@oleibman done

[oleibman]

Thank you. I am sorry to be late with this suggestion, but here goes anyhow. Your test does its job, but it's more complicated than it needs to be. When running unit tests, our default error handler will already convert any warning messages to exceptions. So you don't need to set up your own error handler. You will need to set up a test after you load the spreadsheet, but that should be easy, something like:

$spreadsheet = $reader->load($filename);
$sheet = $spreadsheet->getActiveSheet();
self::assertSame('Report Date', $sheet->getCell('A1')->getValue());
$spreadsheet->disconnectWorksheets();

That test will fail without your change (its warning will be converted to an exception), but will succeed with your change, which is the result we're looking for.

[karrakoliko]

When running unit tests, our default error handler will already convert any warning messages to exceptions

it can be changed any day by any reason.

test should assert that php warning was not triggered.

your test does not: it just reads specific cell value and asserts that it matches expected one, that will be passed even if php warning occurred.

so:

• expected assertion: no php warning triggered
• your test assertion: lib correctly reads value from the cell

if error handler config will be changed - test will be false positive. we dont want potentially false positive tests.

relying on phpunit runtime config parameter instead of clear assertion is a bad idea.

[oleibman]

We have tests to make sure that we treat all warnings, deprecations, and notices as exceptions. That will not be changing. We could not survive new Php releases otherwise. We will not be changing that. On the other hand, Php changes its messages all the time. "Undefined array key 'x'" is by no means guaranteed to be constant.

[karrakoliko]

We have tests to make sure that we treat all warnings, deprecations, and notices as exceptions

ah, this changes a lot.

i'll change test body to yours one and add phpdoc comment about what is actually tested here.

is it ok for you?

[oleibman]

Yes, that would be great. I will merge as soon as I see the change. Thank you for your flexibility.

[oleibman]

@karrakoliko This discussion has been helpful. Although this project does have tests that warnings etc. are converted to exceptions, another project that I work on did not. I will add the necessary tests there.

[karrakoliko]

@oleibman done.
Just to confirm: once this is merged into master, will the patch be included in all versions (3.x, 4.x, 5.x, etc.)?

We’re currently using ^3.5 in production. To suppress that warning we had to add a rather ugly workaround, which we’d like to remove.

[oleibman]

@karrakoliko we don't backport most changes - we just don't have the time. However, I will make an exception in this case - it is a small change, it is unlikely to depend on changes applied to the master branch since it split from the branch that you are using, and the developer (you) wants it. However, I will apply it only to the latest release on your branch, which will be 3.10.2 when I apply it. My plan will be to tag that release just before or just after Php8.5 goes live. If you don't want to try PhpSpreadsheet 5, you should at least consider upgrading to 3.10.1 in the meantime.

[karrakoliko]

Thank you ♥️ @oleibman

[oleibman]

Thank you for your contribution, and for your flexibility. I have also merged this into the realease390 branch.