Skip to content
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

Xml fix namespaces #1756

Closed
wants to merge 55 commits into from
Closed

Conversation

ggiovinazzo
Copy link
Contributor

This is:

- [X] a bugfix
- [ ] a new feature

Checklist:

Why this change is needed?

When reading an Xml/Excel file with namespace defined in header tag and never used (ie: xmlns:x)
The reader fails to load xml file; It tries to access 'x' namespace index but the namespace was never read.

Gianluca Giovinazzo and others added 30 commits August 18, 2020 10:25
The following code generates an error with PHP8:
  if ($t1[1] > 29) {
    $t1[1] += 1900;

Under the "right" conditions, PHP8 evaluates the condition as true
when PHP8 is a non-numeric string and then generates an error trying
to perform += on that non-numeric string. Adding a numeric test
eliminates the problem. All unit tests involving this code now
succeed with both PHP7 and PHP8.
* Reader/Gnumeric Failure with PHP8

An explicit cast from SimpleXML to int is now needed with PHP8,
where it was performed implicitly with PHP7.
Unit tests run correctly for both PHP7 and PHP8 on corrected code.
Some tests of ReferenceHelper functions columnReverseSort and
cellReverseSort which passed with PHP7 fail with PHP8.
Both functions use the following construction:
  return 1 - strcasecmp(whatever);
The "1" seems very mysterious. I believe that the correct code should be:
  return -strcasecmp(whatever);
It appears in particular that PHP7 strcasecmp was never returning a
value of 1 for the tests in question, but PHP8 strcasecmp does so.
With the corrected code, the tests pass in both PHP7 and PHP8.
If font style Superscript is set to true, Subscript is set to false.
Likewise, setting Subscript to true sets Superscript to false.
Both of these are working as they should. However,
setting Superscript to false causes Subscript to be set to true,
and setting Subscript to false causes Superscript to be set to true.
I believe that is an error in both cases. This change fixes it.

There seem to be no existing tests for Font styles.
I added the tests necessary to validate this change.
I will put adding more on my to-do list.
…POffice#1625)

* fixed php8 deprecation warning for libxml_disable_entity_loader()
The plan is to keep Travis for a short while, until we are confident that
GitHub Actions work well enough for us. And after that we can remove Travis
entirely.

There is a bunch of duplicated things but it allows us to maximize
parallelismt to have results as soon as possible.

API documentation generation is still missing.
…try to remove it; nightly should also be allow failures untilany identified issues are resolved
…inst PHP8, because neither library is yet PHP8-ready
MarkBaker and others added 24 commits December 14, 2020 08:14
* Improving Coverage for Excel2003 XML Reader

Reader/Xml is now 100% covered.

File templates/Excel2003XMLTest.xml, used in some tests, is *not*
readable by a current version of Excel. I have substituted a new file
excel2003.xml to be used in its place. I have not deleted the original
in case someone in future (possibly me) wants to see what it needs to
make it usable.

There are minimal code changes.
- Unused protected functions pixel2WidthUnits and widthUnits2Pixel
  are deleted.
- One regex looking to convert hex characters is changed from a-z to a-f,
  and made case insensitive.
- No calculation performed for "error" cell (previously calculation
  was attempted and threw exception).
- Empty relative row/cell is now handled correctly.
- Style applied to empty cell when appropriate.
- Support added for textRotation.
- Support added for border styles.
- Support added for diagonal borders.
- Support added for superscript and subscript.
- Support added for fill patterns.

In theory, encodings other than UTF-8 were supported.
In fact, I was unable to get SecurityScanner to pass *any* xml which is
not UTF-8. Eliminating the assumption that strings might not be UTF-8
allowed much of the code to be greatly simplified.
After that, I added some code that would permit the use of
some ASCII-compatible encodings (there is a test of ISO-8859-1).
It would be more difficult to handle other encodings (such as UTF-16).
I am not convinced that even the ISO-8859 effort is worth it,
but am willing to investigate either expanding or eliminating
non-UTF8 support.

I added a number of tests, creating an Xml directory, and moving
XmlTest to that directory.

Pull Request had problems reading old invalid sample in the code
coverage phase, not in any of the other test phases, and not in
the code coverage phase on my local machine.
As it turns out, aside from being invalid, the sample
is much larger than any of the other samples. Tests have been
adjusted accordingly.

* Smaller Test File

Should eliminate need to avoid test during xml coverage.

* Break Up Style Test into Multiple Tests

Per suggestion from Mark Baker.

* Integrate AddressHelper Change

The introduction of AddressHelper introduced a conflict which needed to
be resolved. I wanted to test it locally before resolving. This required
me to add (unchanged) AddressHelper to my local copy. I hope this is
an okay manner of resolving the conflict.

* Weird Travis Error

XmlOddTest works just fine on my local machine, but Travis failed it.
Even worse, the lines which Travis flags don't even make any sense
(one was the empty line between two methods!).
This test is not essential to the rest of the change. I am removing
it from the package, and will attempt to re-add it when I have a chance
to sync up my fork with the main project.
Coverage data were not correctly uploaded for a long time now, and it
now is uploaded via GitHub Actions
…ding Xlsx Files (PHPOffice#1680)

* Ensure that the list of shared formulae is maintained while chunk-reading Xlsx files
Sheet View Settings Block should be 8-14 bytes long in BIFF8 Excel 97 according
to the open office file format documentation. However access to byte 10 and 12 is
not possible when record data is malformed, so getUInt2d throws notice.
Issue PHPOffice#580 has gone stale since I started work on this.
Nevertheless, this implements an exportArray function as an
exact counterpart of applyFromArry.
I chose the name exportArray to avoid confusion with the existing
method getStyleArray, which does something completely different.

This change also increases coverage for all the Style classes to 100%,
with the exception of Style.php itself. There were several (unchanged)
places in Style.php where I did not have sufficient understanding of
what was supposed to be happening, so could not create tests.

All properties used by applyFromArray are exported by this method.
Note that conditional styles are not covered; this is consistent
with the fact that they are not covered by applyFromArray.

The method is implemented as a final public function in Style/Supervisor,
which calls abstract protected function exportArray1, which is implemented
in each of the subclasses, and which calls final protected
function exportArray2 in Style/Supervisor.
So exportArray is usable for any of the subclasses as well.

The new method is added to the documentation.
The existing documentation for applyFromArray was alphabetized to make
it easier to follow.
One property (Style quotePrefix) was added to the documentation.
Some Borders pseudo-properties (vertical, horizontal, and outline) were
documented as usable by applyFromArray,
but aren't actually supported - they were removed.
The documentation of the properties seemed to use setProperty and
getProperty fairly randomly - it now uses setProperty exclusively.

New constants were added for the textRotation "angles" used to create a
"stacked" cell. I felt that changing the readers and writers to use
these constants was beyond the scope of this change, but it is
on my to-do list.
This will update the function name DCOUNTA from the misspelling of DCOUNT.
All other Samples write to temporary directory. DefinedNames samples
write to main directory, which (a) means they aren't stored with others,
and (b) they aren't ignored by git so look like changed files.
The tests are also simplified by requiring Header rather than Bootstrap,
making use of Helper.
Resolve XSS Vulnerability in the HTML Writer
There are no changes to code. Additional tests are added,
so that the following 6 items now have 100% test coverage:
- Comment
- DefinedName
- DocumentGenerator
- IOFactory
- NamedFormula
- NamedRange
Two changes to fix minor problems reported by Scrutinizer.
@ggiovinazzo ggiovinazzo mentioned this pull request Dec 14, 2020
5 tasks
@oleibman
Copy link
Collaborator

When I run your testNoNameSpaceError script against the current PhpSpreadsheet code, it runs to completion and all the cells have their expected value. Presumably you see an error or a bad value when you try this. Based on the date of your PR (2020-12-14), I think your code base might not have the changes from PR #1698 (merged 2020-12-17), which might (I'm not positive) also have addressed this problem in a different way. Do you still see a problem when you run against the current code, and, if so, what is the problem that you see?

@ggiovinazzo
Copy link
Contributor Author

You're right. The bug has beeen fixed with your commits. I'll close this PR. Thanks.

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.

7 participants