Skip to content

Richtext colors are not being read correctly after namespace change #2458

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

daedeloth
Copy link

@daedeloth daedeloth commented Dec 21, 2021

Related to:
#2303

This is:

  • a bugfix
  • a new feature

Checklist:

Why this change is needed?

…ably related to a namespace change and - according to commit log - issue PHPOffice#2303)
…ably related to a namespace change and - according to commit log - issue PHPOffice#2303)

Use SimpleXMLElement::count() instead of the native count function to avoid code style issues.
@oleibman
Copy link
Collaborator

I agree with your analysis that the namespace change caused the problem, and that the handling of attributes is the specific problem. However, your change is misplaced; RichText is not the only user of ReadColor. The fix belongs in Xlsx/Reader/Styles.php function readColor. On entry, it should have something like:

$attr = Xlsx::getAttributes($color)

Then replace all further uses of $color with $attr in that function. I believe this would solve your problem, and eliminate the failure that occurred when running unit tests on your package.

You should also add a unit test that verifies that the code is working correctly.

@oleibman
Copy link
Collaborator

It looks like a better initialization in function readColor is:

$attr = ($color && $color->attributes()) ? $color->attributes() : [];

oleibman pushed a commit to oleibman/PhpSpreadsheet that referenced this pull request Dec 29, 2021
This is part 2 of a several-phase process to permit PhpSpreadsheet to handle input Xlsx files which use unexpected namespacing. The first phase, introduced as part of release 1.19.0, essentially handled the reading of data. This phase handles the reading of styles. More phases are planned.

It is my intention to leave this in draft status for at least a month. This will give time for additional testing, by me and, I hope, others who might be interested.

This fixes the same problem addressed by PR PHPOffice#2458, if it reaches mergeable status before I am ready to take this out of draft status. I do not anticipate any difficult merge conflicts if the other change is merged first.

This change is more difficult than I'd hoped. I can't get xpath to work properly with the namespaced style file, even though I don't have difficulties with others. Normally we expect:
```xml
<stylesheet xmlns="http://whatever" ...
```
In the namespaced files, we typically see:
```xml
<x:stylesheet xmlns:x="http://whatever" ...
```

Simplexml_load_file specifying a namespace handles the two situations the same, as expected. But, for some reason that I cannot figure out, there are significant differences when xpath processes the result. However, I can manipulate the xml if necessary; I'm not proud of doing that, and will gladly accept any suggestions. In the meantime, it seems to work.

My major non-standard unit test file had disabled any style-related tests when phase 1 was installed. These are now all enabled.
@oleibman oleibman mentioned this pull request Dec 29, 2021
5 tasks
oleibman added a commit that referenced this pull request Feb 11, 2022
* WIP Namespacing Phase 2 - Styles

This is part 2 of a several-phase process to permit PhpSpreadsheet to handle input Xlsx files which use unexpected namespacing. The first phase, introduced as part of release 1.19.0, essentially handled the reading of data. This phase handles the reading of styles. More phases are planned.

It is my intention to leave this in draft status for at least a month. This will give time for additional testing, by me and, I hope, others who might be interested.

This fixes the same problem addressed by PR #2458, if it reaches mergeable status before I am ready to take this out of draft status. I do not anticipate any difficult merge conflicts if the other change is merged first.

This change is more difficult than I'd hoped. I can't get xpath to work properly with the namespaced style file, even though I don't have difficulties with others. Normally we expect:
```xml
<stylesheet xmlns="http://whatever" ...
```
In the namespaced files, we typically see:
```xml
<x:stylesheet xmlns:x="http://whatever" ...
```

Simplexml_load_file specifying a namespace handles the two situations the same, as expected. But, for some reason that I cannot figure out, there are significant differences when xpath processes the result. However, I can manipulate the xml if necessary; I'm not proud of doing that, and will gladly accept any suggestions. In the meantime, it seems to work.

My major non-standard unit test file had disabled any style-related tests when phase 1 was installed. These are now all enabled.

* Scrutinizer

Its analysis is wrong, but the "errors" it pointed out are easy to fix.

* Eliminate XML Source Manipulation

Original solution required XML manipulation to overcome what appears to be an xpath problem. This version replaces xpath with iteration, eliminating the need to manipulate the XML.

* Handle Some Edge Cases

For example, Style file without a Fills section.

* Restore RGB/ARGB Interchangeability

Fix #2494. Apparently EPPlus outputs fill colors as `<fgColor rgb="BFBFBF">` while most output fill colors as `<fgColor rgb="FFBFBFBF">`. EPPlus actually makes more sense. Regardless, validating length of rgb/argb is a recent development for PhpSpreadsheet, under the assumption that an incorrect length is a user error. This development invalidates that assumption, so restore the previous behavior.

In addition, a comment in Colors.php says that the supplied color is "the ARGB value for the colour, or named colour". However, although named colors are accepted, nothing sensible is done with them - they are passed unchanged to the ARGB value, where Excel treats them as black. The routine should either reject the named color, or convert it to the appropriate ARGB value. This change implements the latter.
@oleibman
Copy link
Collaborator

Superseded by #2471. Closing.

@oleibman oleibman closed this Feb 11, 2022
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