Namespacing Phase 2 - Styles #2471
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
|
Scrutinizer reports 3 minor errors. Its analysis is faulty in all 3 cases. A SimpleXMLElement can be falsey, e.g. |
added 3 commits
December 28, 2021 22:58
Its analysis is wrong, but the "errors" it pointed out are easy to fix.
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.
For example, Style file without a Fills section.
Fix PHPOffice#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
changed the title
|
Readied for review after a month in draft status. My plan is to wait at least 2 weeks before merging. |
|
Per my previously-announced schedule, I will attempt to merge this PR tomorrow morning (US Pacific). |
This was referenced Feb 11, 2022
|
Thank you! Sorry for not following up, other stuff got in the way :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
My major non-standard unit test file had disabled any style-related tests when phase 1 was installed. These are now all enabled.
After commit 2d389f5 on Jan. 12, 2022, fix #2494.
This is:
Checklist:
Why this change is needed?