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

Fix last column with relative/absolute flags #607

Closed
wants to merge 9 commits into from
Closed

Fix last column with relative/absolute flags #607

wants to merge 9 commits into from

Conversation

christian-forgacs
Copy link

@christian-forgacs christian-forgacs commented Jul 17, 2018

This is:

  • a bugfix

Checklist:

Why this change is needed?

If you read a xls file and in this file you use an absolut reference in SUM function e.g. =SUM(SHEET!A1:SHEET!B1) the script only returns =SUM(SHEET!A1:SHEET!A1)

Copy link
Member

@PowerKiKi PowerKiKi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing. This unfortunately miss unit tests to be merged. You could add functional tests to test write and read process for this feature.

@christian-forgacs
Copy link
Author

@PowerKiKi the unit tests were not broken by my changes. Currently your develop branch is broken (2 errors from unit tests). I add some unit tests for this bugfix and push it this weekend.

@christian-forgacs
Copy link
Author

christian-forgacs commented Jul 20, 2018

@PowerKiKi can you explain in which case the readBIFF8CellRangeAddressB function is used? In my regular project xls file this function is called by an cell with sum function with a range reference.

I reconstruct the case from this project xls file but always readBIFF8CellRangeAddress is called.

What is the different between tArea and tAreaN? With this information I can create a unit test with an xls file because I can publish the xls from my project.

@christian-forgacs
Copy link
Author

@PowerKiKi I add the unit test for this bugfix and fix the other unit tests with a update for dompdf (from 0.8.0 to 0.8.2).

$calculatedValue = 'empty';
}

throw new \Exception($cellCoordinate . ' != ' . $result . ' - Result in sheet with name ' . $sheetName . ' is ' . $calculatedValue . '.');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a proper assertion such as self::assertSame

*/
public function testLoadXlsWithoutCellReference()
{
$filename = './data/Reader/Xls/without_cell_reference.xls';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We strive to limit the number of binary files in the repository to keep its size down. Can you replace those with by writing them with PhpSpreadsheet and then reloading them instead ? (this is why I suggested functional tests, see \PhpOffice\PhpSpreadsheetTests\Functional\AbstractFunctional)

Copy link
Member

@PowerKiKi PowerKiKi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what DomPDF is doing here, please try to to mix different issues if possible, and remove it from this PR.

@stale
Copy link

stale bot commented Oct 4, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
If this is still an issue for you, please try to help by debugging it further and sharing your results.
Thank you for your contributions.

@stale stale bot added the stale label Oct 4, 2018
@stale stale bot closed this Oct 11, 2018
oleibman added a commit to oleibman/PhpSpreadsheet that referenced this pull request Aug 13, 2024
This supersedes PR PHPOffice#607 by @christian-forgacs, who deserves all the credit for reporting the problem and devising the solution. The PR went stale in 2018, and it is just easier to resubmit a clean version rather than clean up the old one. Among the suggestions in the PR was that you should try to create a spreadsheet from scratch to demonstrate the problem rather than supply one. However, my attempts to match the failing spreadsheet do not have a problem when they are read. So, a supplied spreadsheet it is.

Fix PHPOffice#1570. No sample spreadsheet was supplied with that issue, but I am almost certain that this is another example of the same problem. I am removing the stale label from that issue; it will be closed properly when this PR is merged.
@oleibman
Copy link
Collaborator

Superseded by PR #4140.

@oleibman oleibman removed the stale label Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants