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

CellRange trimmed if from and to is equal, result in auto filter errors #3102

Closed
brainfoolong opened this issue Oct 3, 2022 · 12 comments
Closed
Assignees

Comments

@brainfoolong
Copy link
Contributor

This is:

- [x] a bug report

I have found this line:

if ($this->from->cellAddress() === $this->to->cellAddress()) {

Which effectively convert a range into a single cell, which then causes errors in other functions that explicitely require a range cell name, even of the range consists of just one field.

So the result is, i cannot set AutoFilter with a range of one cell and it will throw an error

Cell get's converted here (__toString is called):

return (string) $cellAddress;

Error thrown here

throw new PhpSpreadsheetException('Autofilter must be set on a range of cells.');

Filter isset with:

public function setAutoFilterByColumnAndRow($columnIndex1, $row1, $columnIndex2, $row2)

The current workaround is to use setAutoFilter instead of setAutoFilterByColumnAndRow and providing a string range.

@MarkBaker
Copy link
Member

I hate to ask, but why would you set autofilter on a single cell?

@brainfoolong
Copy link
Contributor Author

Thanks for answer. To provide a interface where the user can filter, sort, etc... without the need to do anything. It's valid for just one column, or more. Anyway, a single cell works when using setAutoFilter, it's just not working with setAutoFilterByColumnAndRow.

@brainfoolong
Copy link
Contributor Author

brainfoolong commented Oct 3, 2022

It creates that for the user, which is pretty handy, even on a single column.

image

@MarkBaker
Copy link
Member

MarkBaker commented Oct 3, 2022

I'm still not sure that I understand the case for this: I'm trying to create a single cell autofilter in MS Excel itself, but it always seems to adjust it to the full data range

image

You are also aware that the *byColumnAndRow() methods are deprecated?

@brainfoolong
Copy link
Contributor Author

brainfoolong commented Oct 3, 2022

Hm. Ok. That *byColumnAndRow() is deprecated is new to me. Seems that PhpStorm doesn't check that when @Depreceted where it's D is uppercase. Weird :)

Thanks for that excel info. Our internal use cases and codes always only used setAutoFilter with a single row and 1 or X columns. So we can have zero or more rows after.

So we have, for example, to mark row 1 as auto filter, with given numer of columns:

setAutoFilterByColumnAndRow(1, 1, 1, 1);
setAutoFilterByColumnAndRow(1, 1, 3, 1);
setAutoFilterByColumnAndRow(1, 1, 9, 1);

This always worked perfectly fine. However, the case with only one column is indeed pretty rare and can only be when user do some weird settings on the reports. But we never provide more then one row...

Are we doing this wrong, ever since? We have hundreds of phpspreadsheet reports using it like that.

@brainfoolong
Copy link
Contributor Author

I have also tested in my excel, and no errors appear when using a filter settings for that.
Is auto filter in phpspreadsheet not just doing that?

See animation:

screeny

@MarkBaker
Copy link
Member

MarkBaker commented Oct 3, 2022

Defining an AntoFilter in PhpSPreadsheet has always assumed a range of cells, but only ever validated that the provided string matched the expected pattern for a range. I'd never even considered that range might be E4:E4, and I suspect that would break a few of the AutoFilter methods if you tried setting filter values for that range.
It was purely oversimplistic validation that allowed a single cell range to be applied in the first place.

MS Excel actually stores this as a cell reference, and not as a range reference; so it stored E4, and not E4:E4. While I've not tested to confirm it yet, I imagine that most of the AutoFilter methods will throw some exception if they're called on an autofilter defined with only a single row.

But if I remove that validation check, and allowed a single cell reference to match MS Excel, then I see that it will break a few things in the save(), because we also created DefinedNames for the range, and that expects an actual range.

@brainfoolong
Copy link
Contributor Author

Ok, i understand. So, what is your advice on how i should do it? Should i do it, the way i do, defining a range E4:E4 by hand, to overcome the check in autofilter?

@MarkBaker MarkBaker self-assigned this Oct 3, 2022
@MarkBaker
Copy link
Member

For the moment, define as E4:E4 by hand: it will take me a while to identify and find a resolution for all the knock-on effects, initially when saving a file, but also to prevent developers trying to set filters when they can't be set because of a single-cell range.

I'll probably also take a look at how MS Excel auto-adjusts the range as data is added after the autofilter has been set, to see if that's a behaviour that should be replicated.

@brainfoolong
Copy link
Contributor Author

Great. Thanks for your replies.

@MarkBaker
Copy link
Member

If you want to do some testing, take a look at #3111

@brainfoolong
Copy link
Contributor Author

Thanks for the fast respond and code fixes. Unfortunetely i don't have time to test those changes.

MarkBaker added a commit that referenced this issue Dec 21, 2022
### Added

- Extended flag options for the Reader `load()` and Writer `save()` methods
- Apply Row/Column limits (1048576 and XFD) in ReferenceHelper [PR #3213](#3213)
- Allow the creation of In-Memory Drawings from a string of binary image data, or from a stream. [PR #3157](#3157)
- Xlsx Reader support for Pivot Tables [PR #2829](#2829)
- Permit Date/Time Entered on Spreadsheet to be calculated as Float [Issue #1416](#1416) [PR #3121](#3121)

### Changed

- Nothing

### Deprecated

- Direct update of Calculation::suppressFormulaErrors is replaced with setter.
- Font public static variable defaultColumnWidths replaced with constant DEFAULT_COLUMN_WIDTHS.
- ExcelError public static variable errorCodes replaced with constant ERROR_CODES.
- NumberFormat constant FORMAT_DATE_YYYYMMDD2 replaced with existing identical FORMAT_DATE_YYYYMMDD.

### Removed

- Nothing

### Fixed

- Fixed handling for `_xlws` prefixed functions from Office365 [Issue #3245](#3245) [PR #3247](#3247)
- Conditionals formatting rules applied to rows/columns are removed [Issue #3184](#3184) [PR #3213](#3213)
- Treat strings containing currency or accounting values as floats in Calculation Engine operations [Issue #3165](#3165) [PR #3189](#3189)
- Treat strings containing percentage values as floats in Calculation Engine operations [Issue #3155](#3155) [PR #3156](#3156) and [PR #3164](#3164)
- Xlsx Reader Accept Palette of Fewer than 64 Colors [Issue #3093](#3093) [PR #3096](#3096)
- Use Locale-Independent Float Conversion for Xlsx Writer Custom Property [Issue #3095](#3095) [PR #3099](#3099)
- Allow setting AutoFilter range on a single cell or row [Issue #3102](#3102) [PR #3111](#3111)
- Xlsx Reader External Data Validations Flag Missing [Issue #2677](#2677) [PR #3078](#3078)
- Reduces extra memory usage on `__destruct()` calls [PR #3092](#3092)
- Additional properties for Trendlines [Issue #3011](#3011) [PR #3028](#3028)
- Calculation suppressFormulaErrors fix [Issue #1531](#1531) [PR #3092](#3092)
- Permit Date/Time Entered on Spreadsheet to be Calculated as Float [Issue #1416](#1416) [PR #3121](#3121)
- Incorrect Handling of Data Validation Formula Containing Ampersand [Issue #3145](#3145) [PR #3146](#3146)
- Xlsx Namespace Handling of Drawings, RowAndColumnAttributes, MergeCells [Issue #3138](#3138) [PR #3136](#3137)
- Generation3 Copy With Image in Footer [Issue #3126](#3126) [PR #3140](#3140)
- MATCH Function Problems with Int/Float Compare and Wildcards [Issue #3141](#3141) [PR #3142](#3142)
- Fix ODS Read Filter on number-columns-repeated cell [Issue #3148](#3148) [PR #3149](#3149)
- Problems Formatting Very Small and Very Large Numbers [Issue #3128](#3128) [PR #3152](#3152)
- XlsxWrite preserve line styles for y-axis, not just x-axis [PR #3163](#3163)
- Xlsx Namespace Handling of Drawings, RowAndColumnAttributes, MergeCells [Issue #3138](#3138) [PR #3137](#3137)
- More Detail for Cyclic Error Messages [Issue #3169](#3169) [PR #3170](#3170)
- Improved Documentation for Deprecations - many PRs [Issue #3162](#3162)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants