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

SYLK shared formulae #2267

Closed
SheetJSDev opened this issue Aug 10, 2021 · 2 comments · Fixed by #3776
Closed

SYLK shared formulae #2267

SheetJSDev opened this issue Aug 10, 2021 · 2 comments · Fixed by #3776

Comments

@SheetJSDev
Copy link
Contributor

This is:

- [X] a bug report
- [ ] a feature request
- [X] **not** a usage question (ask them on https://stackoverflow.com/questions/tagged/phpspreadsheet or https://gitter.im/PHPOffice/PhpSpreadsheet)

This SYLK file uses the shared formula optimization that is present in XLS and XLSX:

ID;PWXL;N;E
P;PGeneral
F;P0;DG0G10;M320
B;Y3;X1;D0 0 9 0
C;Y1;X1;K1
C;Y2;K2;ER[-1]C+1
C;Y3;K3;S;R2;C1
E

Excel understands this as a worksheet with A1 set to 1, A2 set to A1+1 (cached value 2), A3 set to A2+1 (cached value 3)

PHPSpreadsheet incorrectly handles the A3 line, setting A2 to 3, because of an error in Slk.php:

            switch ($rowDatum[0]) {
                case 'C': // <-- should be removed
                case 'X':
                    $column = substr($rowDatum, 1);

                    break;
                case 'R': // <-- should be removed
                case 'Y':
                    $row = substr($rowDatum, 1);

The code appears to assume that C and R refer to the cell but it in fact refers to the referenced cell. It will attempt to set A2 to the value 3.

Unfortunately there's no official spec for any of this, but it appears that the C record supports the following fields:

  • S: signals that the cell is using a shared formula

  • R and C: row/column of the cell with the formula (1-indexed numeric values)

What is the expected behavior?

print_r($sheet->getCell("A2")->getValue()); should print 2

What is the current behavior?

print_r($sheet->getCell("A2")->getValue()); prints 3

What are the steps to reproduce?

Save the SYLK codeblock to test.slk and run

<?php
require 'vendor/autoload.php';
use PhpOffice\PhpSpreadsheet\Spreadsheet;

$spreadsheet = \PhpOffice\PhpSpreadsheet\IOFactory::load("test.slk");
$sheet = $spreadsheet->getActiveSheet();
print_r($sheet->getCell("A2")->getValue());

Which versions of PhpSpreadsheet and PHP are affected?

All versions of PHPSpreadsheet (error inherited from PHPExcel) and every commit of PHPExcel on github (blamelog points to PHPOffice/PHPExcel@509f27e )

@oleibman
Copy link
Collaborator

Excellent documentation - thanks. However, a little more investigation is needed. The result of getValue on cell A2 should not be 2; it should be the formula =A1+1 (or, in Sylk terms, =R[-1]C+1). The result of getCalculatedValue on A2 should be 2. Your suggested change produces both of those results correctly. However, cell A3, which correctly gives a calculated value of 3 after your change, also gives 3 as the result of getValue, and it should be giving =A2+1 (or the same formula as for A2 in Sylk terms). I'm not sure how I get from C;Y3;K3;S;R2;C1 to that. It seems to indicate using the same formala as for cell A2 (R2;C1), which is good if the formula is still available in Sylk terms, but not so good if, as I fear, the formula is available at this point only in Excel terms. So, the ultimate fix will be more complicated than you imagined, and may take a while to figure out.

@SheetJSDev
Copy link
Contributor Author

PHPSpreadsheet already implemented the shared formula logic for XLSX (https://github.com/PHPOffice/PhpSpreadsheet/blob/master/src/PhpSpreadsheet/Reader/Xlsx.php#L320 is the relevant part) and in XLS.

To handle the shared formula case: track whether the "S" field is present and separately store the R and C values. If "S" is found, use the R and C values to find the base cell, grab the formula, then shift to the current address.

In our example C;Y3;K3;S;R2;C1, the S field indicates that this is shared. R2 is saying the base cell is on row 2 and C1 is saying the base cell is in column A. So the base cell is "A2". Then grab the formula from A2 and shift to the current address.

If the RC-formula is stored (R[-1]C+1) the library can just convert using A3 as the base cell (yielding A2+1)

If the A1-formula is stored (A1+1), updateFormulaReferences should be used to perform the shift

oleibman added a commit to oleibman/PhpSpreadsheet that referenced this issue Oct 25, 2023
Fix PHPOffice#2267. The Slk format has a way to express a "shared formula", but the Slk reader does not yet understand it. Thanks to @SheetJSDev for documenting the problem and pointing the way towards a solution. It has taken a long time to get there. Part of the problem is that I have not been successful in getting Excel to use this type of construction when saving a Slk file. So I have resorted to saving a Slk file where shared formulas *could* be used, and then editing it by hand to actually use them. It would not surprise me in the least to have neglected one or more possible ways to specify a shared formula; but, at least the issue as documented is resolved, and if new issues arise, we'll probably be in better shape to deal with them.
@oleibman oleibman mentioned this issue Oct 25, 2023
11 tasks
oleibman added a commit that referenced this issue Oct 30, 2023
* Slk Shared Formulas

Fix #2267. The Slk format has a way to express a "shared formula", but the Slk reader does not yet understand it. Thanks to @SheetJSDev for documenting the problem and pointing the way towards a solution. It has taken a long time to get there. Part of the problem is that I have not been successful in getting Excel to use this type of construction when saving a Slk file. So I have resorted to saving a Slk file where shared formulas *could* be used, and then editing it by hand to actually use them. It would not surprise me in the least to have neglected one or more possible ways to specify a shared formula; but, at least the issue as documented is resolved, and if new issues arise, we'll probably be in better shape to deal with them.

* Update CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants