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

Save Excel 2010+ Functions Properly #1461

Closed
wants to merge 3 commits into from
Closed

Save Excel 2010+ Functions Properly #1461

wants to merge 3 commits into from

Conversation

oleibman
Copy link
Collaborator

@oleibman oleibman commented May 3, 2020

This is:

- [x] a bugfix
- [ ] a new feature

Checklist:

Why this change is needed?

For functions introduced in Excel 2010 and beyond, Excel saves them
in formulas with the xlfn_ prefix. PhpSpreadsheet does not do this;
as a result, when a spreadsheet so created is opened, the cells
which use the new functions display a #NAME? error.
This the cause of bug report 1246:
#1246
This change corrects that problem when the Xlsx writer encounters
a 2010+ formula for a cell or a conditional style. A new class
Writer/Xlsx/Xlfn, with 2 static methods,
is introduced to facilitate this change.

As part of the testing for this, I found some additional problems.
When an unknown function name is used, Excel generates a #NAME? error.
However, when an unknown function is used in PhpSpreadsheet:

  • if there are no parameters, it returns #VALUE!, which is wrong
  • if there are parameters, it throws an exception, which is horrible
    Both of these situations will now return #NAME?
    Tests have been added for these situations.

The MODE (and MODE.SNGL) function is not quite in alignment with Excel.
MODE(3, 3, 4, 4) returns 3 in both Excel and PhpSpreadsheet.
However, MODE(4, 3, 3, 4) returns 4 in Excel, but 3 in PhpSpreadsheet.
Both situations will now match Excel's result.
Also, Excel allows its parameters for MODE to be an array,
but PhpSpreadsheet did not; it now will.
There had not been any tests for MODE. Now there are.

The SHEET and SHEETS functions were introduced in Excel 2013,
but were not introduced in PhpSpreadsheet. They are now introduced
as DUMMY functions so that they can be parsed appropriately.

Finally, in common with the "rate" changes for which I am
creating a pull request at the same time as this one:
samples/Basic/13_CalculationCyclicFormulae
PhpUnit started reporting an error like "too much regression".
The test deals with an infinite cyclic formula, and allowed
the calculation engine to run for 100 cycles. The actual number of cycles
seems irrelevant for the purpose of this test. I changed it to 15,
and PhpUnit no longer complains.

Resync with base project
For functions introduced in Excel 2010 and beyond, Excel saves them
in formulas with the xlfn_ prefix. PhpSpreadsheet does not do this;
as a result, when a spreadsheet so created is opened, the cells
which use the new functions display a #NAME? error.
This the cause of bug report 1246:
#1246
This change corrects that problem when the Xlsx writer encounters
a 2010+ formula for a cell or a conditional style. A new class
Writer/Xlsx/Xlfn, with 2 static methods,
is introduced to facilitate this change.

As part of the testing for this, I found some additional problems.
When an unknown function name is used, Excel generates a #NAME? error.
However, when an unknown function is used in PhpSpreadsheet:
  - if there are no parameters, it returns #VALUE!, which is wrong
  - if there are parameters, it throws an exception, which is horrible
Both of these situations will now return #NAME?
Tests have been added for these situations.

The MODE (and MODE.SNGL) function is not quite in alignment with Excel.
MODE(3, 3, 4, 4) returns 3 in both Excel and PhpSpreadsheet.
However, MODE(4, 3, 3, 4) returns 4 in Excel, but 3 in PhpSpreadsheet.
Both situations will now match Excel's result.
Also, Excel allows its parameters for MODE to be an array,
but PhpSpreadsheet did not; it now will.
There had not been any tests for MODE. Now there are.

The SHEET and SHEETS functions were introduced in Excel 2013,
but were not introduced in PhpSpreadsheet. They are now introduced
as DUMMY functions so that they can be parsed appropriately.

Finally, in common with the "rate" changes for which I am
creating a pull request at the same time as this one:
samples/Basic/13_CalculationCyclicFormulae
PhpUnit started reporting an error like "too much regression".
The test deals with an infinite cyclic formula, and allowed
the calculation engine to run for 100 cycles. The actual number of cycles
seems irrelevant for the purpose of this test. I changed it to 15,
and PhpUnit no longer complains.
One call which was static should be object call.
@PowerKiKi
Copy link
Member

Merged in 4f6d4af, thanks !

@PowerKiKi PowerKiKi closed this May 18, 2020
@oleibman oleibman deleted the xlfnchanges branch May 19, 2020 04:08
PowerKiKi added a commit that referenced this pull request May 31, 2020
### Added

- Support writing to streams in all writers [#1292](#1292)
- Support CSV files with data wrapping a lot of lines [#1468](#1468)
- Support protection of worksheet by a specific hash algorithm [#1485](#1485)

### Fixed

- Fix Chart samples by updating chart parameter from 0 to DataSeries::EMPTY_AS_GAP [#1448](#1448)
- Fix return type in docblock for the Cells::get() [#1398](#1398)
- Fix RATE, PRICE, XIRR, and XNPV Functions [#1456](#1456)
- Save Excel 2010+ functions properly in XLSX [#1461](#1461)
- Several improvements in HTML writer [#1464](#1464)
- Fix incorrect behaviour when saving XLSX file with drawings [#1462](#1462),
- Fix Crash while trying setting a cell the value "123456\n" [#1476](#1481)
- Improved DATEDIF() function and reduced errors for Y and YM units [#1466](#1466)
- Stricter typing for mergeCells [#1494](#1494)

### Changed

- Drop support for PHP 7.1, according to https://phpspreadsheet.readthedocs.io/en/latest/#php-version-support
- Drop partial migration tool in favor of complete migration via RectorPHP [#1445](#1445)
- Limit composer package to `src/` [#1424](#1424)
@tyteen4a03
Copy link

This fixes ISOWEEKNUM() but doesn't fix DAYS(). Is DAYS() related to this change?

@oleibman
Copy link
Collaborator Author

oleibman commented Jul 8, 2020

Yes, DAYS is covered by this change. The following code produces the expected result for me when run against a freshly downloaded copy of PhpSpreadsheet:
$spreadsheet = new Spreadsheet();
$sheet = $spreadsheet->getActiveSheet();
$sheet->setCellValue('A1', '=DAYS("2017-01-01","2016-01-01")');
$writer = IOFactory::createWriter($spreadsheet, 'Xlsx');
$writer->save('outfile.xlsx');
What kind of problem are you seeing?

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.

3 participants