Fix offset value on xls #2312
Closed
Fix offset value on xls #2312
Conversation
This file contains 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
hannanyusop
commented
Oct 3, 2021
hannanyusop
commented
added 30 commits
October 30, 2020 10:48
…rguments and returns) and that all argument names for public methods are meaningful (in readiness for PHP8 named arguments)
# Conflicts: # src/PhpSpreadsheet/Style/Alignment.php # src/PhpSpreadsheet/Style/Color.php
My bulletproofing of these tests was not yet sufficient. Although I have never had a failure in probably thousands of tests, one user submitted a PR which did fail testing NOW, fortunately not in a test that is required to pass. The problem is that it is not sufficient merely to set the cell value inside a do-while loop; it is necessary to calculate it in order to cache its result so that results based on that cell will be internally consistent. No source code is changed for this PR, just some tests.
|
Now it's 36 commits and 97 files. Perhaps you should just clone and create a new branch with the one change (and test files). |
oleibman
added a commit
to oleibman/PhpSpreadsheet
that referenced
this pull request
Oct 16, 2021
See issue PHPOffice#2315. It is nominally solved by PR PHPOffice#2312, but that PR is completely unsuitable for merging. This one-line change is a replacement for that PR. As with many problems of this type, it is not clear how how to create a spreadsheet with this sort of harmless corruption in the wild. An example was supplied with the issue, and I have tested manually against it. However, the file is huge and not suitable for a formal unit test. I do not understand BIFF well enough to try and craft a suitable example on my own.
This change was suggested by issue #2316. There was a problem reading Xlsx comments which appeared with release 18.0 but which was already fixed in master. So no source change was needed to fix the issue, but I thought we should at least add the test case to our unit tests. In developing that case, I discovered that, although comment text was read correctly, there was a problem with comment author. In fact, there were two problems. One was new, with the namespacing changes - as in several other cases, the namespaced attribute `authorId` needed some special handling. However, another problem was much older - the code was checking `!empty($comment['authorId'])`, eliminating consideration of authorId=0, and should instead have been checking `isset`. Both problems are now fixed, and tested.
See issue #2123. HLOOKUP needs to do some conversions between column numbers and letters which it had not been doing. HLOOKUP tests were performed using direct calls to the function in question rather than in the context of a spreadsheet. This contributed to keeping this error obscured even though there were, in theory, sufficient test cases. The tests are changed to perform in spreadsheet context. For the most part, the test cases are unchanged. One of the expected results was wrong; it has been changed, and a new case added to cover the case it was supposed to be testing. After getting the HLOOKUP tests in order, it turned out that a test using literal arrays which had been succeeding now failed. The array constructed by the literals are considerably different than those constructed using spreadsheet cells; additional code was added to handle this situation.
See issue #2331. Timestamp is expected in format yyyy-mm-dd (plus other information), with the expectation that month and day are 2 digits zero-filled on the left if needed. The user's file instead used a space rather than zero as filler. Although I don't know how the unexpected timestamp was created, it was easy enough to alter the timestamp in an otherwise normal spreadsheet, and use that file as a test case.
Co-authored-by: Adrien Crivelli <[email protected]>
Preparatory to new release expected soon.
This changes is to clarify that the sheet title is used to reference the sheet and that the title must be single quoted.
The optimization from #773 was not copied along in #1033, so restore it Co-authored-by: Adrien Crivelli <[email protected]>
Prevent calling clone and getHashCode when not needed because these calls are very expensive. When applying styles to a range of cells can we cache the styles we encounter along the way so we don't need to look them up with getHashCode later.
Final stage of preparation for PHP8
fixed in the same way as it already has been done for strToLower Co-authored-by: Adrien Crivelli <[email protected]>
|
Not sure what happened here, but this won't be merged as is. Please re-create a new PR with correct branch. |
|
Actually, this is already recreated in #2338 |
oleibman
added a commit
that referenced
this pull request
Nov 2, 2021
See issue #2315. It is nominally solved by PR #2312, but that PR is completely unsuitable for merging. This one-line change is a replacement for that PR. As with many problems of this type, it is not clear how how to create a spreadsheet with this sort of harmless corruption in the wild. An example was supplied with the issue, and I have tested manually against it. However, the file is huge and not suitable for a formal unit test. I do not understand BIFF well enough to try and craft a suitable example on my own. Co-authored-by: Adrien Crivelli <[email protected]>
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.