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

Uncaught TypeError: Argument 2 passed to PhpOffice\PhpSpreadsheet\Calculation\LookupRef::INDIRECT() must be an instance of PhpOffice\PhpSpreadsheet\Cell\Cell or null, bool given #1704

Closed
fami-64 opened this issue Nov 6, 2020 · 1 comment · Fixed by #3052

Comments

@fami-64
Copy link

fami-64 commented Nov 6, 2020

This is:

- [x] a bug report

What is the expected behavior?

No error when reading a cell with $cell->getCalculatedValue() (from a XLSX file)

What is the current behavior?

Fatal error: Uncaught TypeError: Argument 2 passed to PhpOffice\PhpSpreadsheet\Calculation\LookupRef::INDIRECT() must be an instance of PhpOffice\PhpSpreadsheet\Cell\Cell or null, bool given in .../phplib/PhpOffice/PhpSpreadsheet/Calculation/LookupRef.php:276 Stack trace: #0 [internal function]: PhpOffice\PhpSpreadsheet\Calculation\LookupRef::INDIRECT('L1C2', false, Object(PhpOffice\PhpSpreadsheet\Cell\Cell)) #1 .../phplib/PhpOffice/PhpSpreadsheet/Calculation/Calculation.php(4714): call_user_func_array(Array, Array) #2 .../phplib/PhpOffice/PhpSpreadsheet/Calculation/Calculation.php(3442): PhpOffice\PhpSpreadsheet\Calculation\Calculation->processTokenStack(Array, 'A1', Object(PhpOffice\PhpSpreadsheet\Cell\Cell)) #3 .../phplib/PhpOffice/PhpSpreadsheet/Calculation/Calculation.php(3233): PhpOffice\PhpSpreadsheet\Calculation\Calculation->_calculateFormulaValue('INDIRECT("L1C2"...', 'A1', Object(PhpOffice\PhpSprea in .../phplib/PhpOffice/PhpSpreadsheet/Calculation/LookupRef.php on line 276

What are the steps to reproduce?

Create a xlsx file with a formula like "=indirect('L1C2', FAUX)" in the first cell
Nota : I use a French version of Excel

Please provide a Minimal, Complete, and Verifiable example of code that exhibits the issue without relying on an external Excel file or a web server:

<?php

require '../phplib/autoload.php';
use PhpOffice\PhpSpreadsheet\IOFactory;

$inputFileName = 'a/valid/xlsx/file';
$reader = IOFactory::createReader('Xlsx');
$spreadsheet = $reader->load($inputFileName);
$worksheet = $spreadsheet->getSheet(0);

// add code that show the issue here...
foreach ($worksheet->getRowIterator() as $row) {
	$cellIterator = $row->getCellIterator();
	$cellIterator->setIterateOnlyExistingCells(FALSE);
	foreach ($cellIterator as $cell) echo '<br/>' . $cell->getCalculatedValue();
}

When I use "getValue()" rather than "getCalculatedValue", it works and returns "=INDIRECT("L1C2",FALSE)"
test.xlsx
test.xlsx

Which versions of PhpSpreadsheet and PHP are affected?

PhpSpreadsheet 1.15.0
PHP v7.4.3

@oleibman
Copy link
Collaborator

oleibman commented Sep 4, 2022

It still produces an error, though it is now easier to understand:

Fatal error: Uncaught PhpOffice\PhpSpreadsheet\Calculation\Exception: Feuil1!A1
-> Invalid R1C1-format Cell Reference in C:\git\dateax\src\PhpSpreadsheet\Cell\Cell.php on line 298

It is worth noting that on my (US English) version of Excel, the cell in question displays #REF!, so your spreadsheet isn't portable even in Excel. This makes it difficult to decide what the best way for PhpSpreadsheet to handle it would be.

oleibman added a commit to oleibman/PhpSpreadsheet that referenced this issue Sep 5, 2022
Fix PHPOffice#1704, albeit imperfectly. Excel's implementation of this feature makes it impossible to fix perfectly. I don't know why it was necessary to internationalize R1C1 in the first place - the benefits are so minimal,and the result is worksheets that break when opened in different locales. Ugh. I can't even find complete documentation about the format in different languages; I am using https://answers.microsoft.com/en-us/officeinsider/forum/all/indirect-function-is-broken-at-least-for-excel-in/1fcbcf20-a103-4172-abf1-2c0dfe848e60 as my definitive reference.

This fix concentrates on the original report, using the INDIRECT function; there may be other areas similarly affected. As with ambiguous date formats, PhpSpreadsheet will do a little better than Excel itself when reading spreadsheets with internationalized R1C1 by trying all possibilities before giving up. When it does give up, it will now return `#REF!`, as Excel does, rather than throwing an exception, which is certainly friendlier. Although read now works better, when writing it will use whatever the user specified, so spreadsheets breaking in the wrong locale will still happen.

There were some bugs that turned up as I added test cases, all of them concerning relative addressing in R1C1 format, e.g. `R[+1]C[-1]`. The regexp for validating the format allowed for minus signs, but not plus signs. Also, the relevant functions did not allow for passing the current cell address, which made relative addressing impossible. The code now allows these, and suitable test cases are added.
oleibman added a commit that referenced this issue Sep 16, 2022
* R1C1 Format and Internationalization, plus Relative Offsets

Fix #1704, albeit imperfectly. Excel's implementation of this feature makes it impossible to fix perfectly. I don't know why it was necessary to internationalize R1C1 in the first place - the benefits are so minimal,and the result is worksheets that break when opened in different locales. Ugh. I can't even find complete documentation about the format in different languages; I am using https://answers.microsoft.com/en-us/officeinsider/forum/all/indirect-function-is-broken-at-least-for-excel-in/1fcbcf20-a103-4172-abf1-2c0dfe848e60 as my definitive reference.

This fix concentrates on the original report, using the INDIRECT function; there may be other areas similarly affected. As with ambiguous date formats, PhpSpreadsheet will do a little better than Excel itself when reading spreadsheets with internationalized R1C1 by trying all possibilities before giving up. When it does give up, it will now return `#REF!`, as Excel does, rather than throwing an exception, which is certainly friendlier. Although read now works better, when writing it will use whatever the user specified, so spreadsheets breaking in the wrong locale will still happen.

There were some bugs that turned up as I added test cases, all of them concerning relative addressing in R1C1 format, e.g. `R[+1]C[-1]`. The regexp for validating the format allowed for minus signs, but not plus signs. Also, the relevant functions did not allow for passing the current cell address, which made relative addressing impossible. The code now allows these, and suitable test cases are added.

* Use Locale for Formats, but Not for XML

Implementing a suggestion from @MarkBaker to use the system locale for determining R1C1 format rather than looping through a set of regexes and accepting any that work. This is closer to how Excel itself operates. The assumption we are making is to use the first character of the translated ROW and COLUMN functions. This will not work for Russian or Bulgarian, where each starts with the same letter, but it appears that Russian, at least, still uses R1C1. So our algorithm will not use non-ASCII characters, nor characters where ROW and COLUMN start with the same letter, falling back to R/C in those cases. Turkish falls into that category. Czech uses an accented character for one of the functions, and I'm guessing to use the unaccented character in that case. Polish COLUMN function is NR.KOLUMNY, and I'm guessing to use K in that case.

The function that converts R1C1 references is also used by the XML reader *where the format is always R1C1*, not locale-based (confirmed by successfully opening in Excel an XML spreadsheet when my language is set to French). The conversion code now handles that distinction through the use of an extra parameter. Xml Reader Load Test is duplicated to confirm that spreadsheet is loaded properly whether the locale is English or French. (No, I did not add an INDIRECT function to the Xml spreadsheet.)

Tests CsvIssue2232Test and TranslationTest both changed locale without resetting it when done. That omission was exposed by the new code, and both are now corrected.

* OpenOffice and Gnumeric

OpenOffice and Gnumeric make it much easier to test with other languages - they can be handled with an environment variable. Sensibly, they require R and C as the characters for R1C1 notation regardless of the language. Change code to recognize this difference from Excel.

* Handle Output of ADDRESS Function

One other function has to deal with R1C1 format as a string. Unlike INDIRECT, which receives the string on input, ADDRESS generates the string on output. Ensure that the ADDRESS output is consistent with the INDIRECT input.

ADDRESS expects its 4th arg to be bool, but it can also accept int, and many examples on the net supply it as an int. This had not been handled properly, but is now corrected.

* More Structured Test

I earlier introduced a new test for relative R1C1 addressing. Rewrite it to be clearer.

* Add Row for This to Locale Spreadsheet

It took a while for me to figure out how it all works. I have added a new row (with English value `*RC`) to Translations.xlsx, in the "Lookup and Reference" section of sheet "Excel Functions". By starting the "function name" with an asterisk, it will not be confused with a "real" function (confirmed by a new test). This approach also gives us the flexibility to do something similar if another surprise case occurs in future; in particular, I think this is more flexible than adding this as another option on the "Excel Localisation" sheet. It also means that any errors or omissions in the list below will be handled as with any other translation problem, by updating the spreadsheet without needing to touch any code.

The spreadsheet has the following entries in the *RC row:
- first letter of ROW/COLUMN functions for da, de, es, fi, fr, hu, nl, nb, pt, pt_br, sv
- no value for locales where ROW/COLUMN functions start with same letter - bg, ru, tr
- no value for locales with a multi-part name for ROW and/or COLUMN - it, pl (I had not previously noted Italian as an exception)
- no value for locales where ROW and/or COLUMN starts with a non-ASCII character - cs (this would also apply to bg and ru which are already included under "same letter")
- it does nothing for locales which are defined on the "Excel Localisation" sheet but have no entries yet on the "Excel Functions" sheet (e.g. eu)

Note that all but the first bullet item will continue to use R/C, which leaves them no worse off than they were before this change.
MarkBaker added a commit that referenced this issue Sep 25, 2022
### Added

- Implementation of the new `TEXTBEFORE()`, `TEXTAFTER()` and `TEXTSPLIT()` Excel Functions
- Implementation of the `ARRAYTOTEXT()` and `VALUETOTEXT()` Excel Functions
- Support for [mitoteam/jpgraph](https://packagist.org/packages/mitoteam/jpgraph) implementation of
  JpGraph library to render charts added.
- Charts: Add Gradients, Transparency, Hidden Axes, Rounded Corners, Trendlines, Date Axes.

### Changed

- Allow variant behaviour when merging cells [Issue #3065](#3065)
  - Merge methods now allow an additional `$behaviour` argument. Permitted values are:
    - Worksheet::MERGE_CELL_CONTENT_EMPTY - Empty the content of the hidden cells (the default behaviour)
    - Worksheet::MERGE_CELL_CONTENT_HIDE - Keep the content of the hidden cells
    - Worksheet::MERGE_CELL_CONTENT_MERGE - Move the content of the hidden cells into the first cell

### Deprecated

- Axis getLineProperty deprecated in favor of getLineColorProperty.
- Moved majorGridlines and minorGridlines from Chart to Axis. Setting either in Chart constructor or through Chart methods, or getting either using Chart methods is deprecated.
- Chart::EXCEL_COLOR_TYPE_* copied from Properties to ChartColor; use in Properties is deprecated.
- ChartColor::EXCEL_COLOR_TYPE_ARGB deprecated in favor of EXCEL_COLOR_TYPE_RGB ("A" component was never allowed).
- Misspelled Properties::LINE_STYLE_DASH_SQUERE_DOT deprecated in favor of LINE_STYLE_DASH_SQUARE_DOT.
- Clone not permitted for Spreadsheet. Spreadsheet->copy() can be used instead.

### Removed

- Nothing

### Fixed

- Fix update to defined names when inserting/deleting rows/columns [Issue #3076](#3076) [PR #3077](#3077)
- Fix DataValidation sqRef when inserting/deleting rows/columns [Issue #3056](#3056) [PR #3074](#3074)
- Named ranges not usable as anchors in OFFSET function [Issue #3013](#3013)
- Fully flatten an array [Issue #2955](#2955) [PR #2956](#2956)
- cellExists() and getCell() methods should support UTF-8 named cells [Issue #2987](#2987) [PR #2988](#2988)
- Spreadsheet copy fixed, clone disabled. [PR #2951](#2951)
- Fix PDF problems with text rotation and paper size. [Issue #1747](#1747) [Issue #1713](#1713) [PR #2960](#2960)
- Limited support for chart titles as formulas [Issue #2965](#2965) [Issue #749](#749) [PR #2971](#2971)
- Add Gradients, Transparency, and Hidden Axes to Chart [Issue #2257](#2257) [Issue #2229](#2929) [Issue #2935](#2935) [PR #2950](#2950)
- Chart Support for Rounded Corners and Trendlines [Issue #2968](#2968) [Issue #2815](#2815) [PR #2976](#2976)
- Add setName Method for Chart [Issue #2991](#2991) [PR #3001](#3001)
- Eliminate partial dependency on php-intl in StringHelper [Issue #2982](#2982) [PR #2994](#2994)
- Minor changes for Pdf [Issue #2999](#2999) [PR #3002](#3002) [PR #3006](#3006)
- Html/Pdf Do net set background color for cells using (default) nofill [PR #3016](#3016)
- Add support for Date Axis to Chart [Issue #2967](#2967) [PR #3018](#3018)
- Reconcile Differences Between Css and Excel for Cell Alignment [PR #3048](#3048)
- R1C1 Format Internationalization and Better Support for Relative Offsets [Issue #1704](#1704) [PR #3052](#3052)
- Minor Fix for Percentage Formatting [Issue #1929](#1929) [PR #3053](#3053)
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