Skip to content

Last Sheet is alway set as the Active Sheet upon loading the File #2666

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

Closed
eXsio opened this issue Mar 11, 2022 · 6 comments
Closed

Last Sheet is alway set as the Active Sheet upon loading the File #2666

eXsio opened this issue Mar 11, 2022 · 6 comments

Comments

@eXsio
Copy link

eXsio commented Mar 11, 2022

This is:

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

What is the expected behavior?

According to the documentation Worksheet::getActiveSheet() should always return "the one that will be active when the workbook is opened in MS Excel (or other appropriate Spreadsheet program)."

What is the current behavior?

Currenty, the last Sheet is always marked as the Active Sheet after the file was loaded:

  1. Worksheet:676 iterates over the file's Sheets
  2. Worksheet:713 loads conditional styles for the currently processed Sheet
  3. ConditionalStyles:36 delegates the loading to the setConditionalStyles method
  4. ConditionalStyles:174 calls Worksheet::getStyle method
  5. Worksheet:1422 calls setActiveSheetIndex method

Assuming that all the Sheets are always loaded and that they are loaded in the order of the file's Sheets, the last Sheet in the file will always be set as the Active Sheet.

This functionality was working correctly in version 1.7.0 which I'm moving from. I've tried to downgrade to 1.15.0 but the issue is also there, so it's been in the code for quite some time. Versions below 1.15.0 are not compatible with PHP 8.0 so I can't go lower. This is a major blocker for me.

The root cause for me is that get getStyle method changes the Active Sheet Index. Generally methods that start with "get" shouldn't change the state of any objects.

Proposed solutions:

  1. Change the getStyle method to not change the data state
  2. Detect and cache the Active Sheet Index prior to any loading and re-set it once the load has been completed

What are the steps to reproduce?

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 __DIR__ . '/vendor/autoload.php';

// Load Spreadsheet from file that contains multiple Sheets, and the Active Sheet Index in the file != Last Sheet Index of the file
$spreadsheet = IOFactory::load($filename);

//Load the Active Sheet - it should be the actually Active Sheet, but instead it is the Last Sheet
 $worksheet = spreadsheet->getActiveSheet();

### Which versions of PhpSpreadsheet and PHP are affected?
- 8.0
- 8.1
@eXsio eXsio changed the title Last Sheet is alway set as the Active Sheet upon reading Last Sheet is alway set as the Active Sheet upon loading the File Mar 11, 2022
@pich
Copy link

pich commented Mar 11, 2022

+1

@MarkBaker
Copy link
Member

What version of PhpSpreadsheet? And what file formats are affected? And if you have a sample file that demonstrates this problem, that would be useful.

I've created a spreadsheet with four worksheets; added a conditional style in the 2nd sheet; and set the first sheet as the active sheet.

I've tested this for both the master branch and the 1.22.0 release, against PHP versions 7.4.26, PHP 8.0.13 and PHP 8.1.0 and it works as expected for Xlsx, Xls and Ods files; setting the active sheet on loading a Gnumeric file is a new addition to the code that hasn't yet been released.

I've repeated that exercise with a new spreadsheet with four worksheets, with the CF on the 3rd sheet, and the 2nd worksheet as the active sheet; again, this works as expected for Xlsx and Xls and Ods.

@eXsio
Copy link
Author

eXsio commented Mar 11, 2022

Hi, thank you for a quick reply. I've tested versions from 1.15 to 1.22. Here's a full, minimal, reproducible example: https://drive.google.com/file/d/1matrA-VjujGJ_MUBwrxleTmBaT4JIcJR/view?usp=sharing

Just run 'composer install' an then 'php reproduction.php'

@MarkBaker
Copy link
Member

Interesting.
The current active tab is held in the bookViews element of the workbook.xml; if no activeTab attribute is defined in that element, then PhpSpreadsheet defaults to the first worksheet loaded from the spreadsheet file. This can happen if activeTab attribute isn't set (Excel doesn't bother setting the attribute if the active tab is the first worksheet), or if the worksheet that it refers to wasn't loaded (e.g. if the Reader was told only to load specific worksheets).

However, it gets more interesting in the case of your file (from Google Sheets), where the bookViews element doesn't exist at all. In that case, it bypasses PhpSpreadsheet's logic for setting the active worksheet completely, which means that it is left set to the last worksheet that was loaded.

It should be easy enough for me to provide a default to the first worksheet in that circumstance.

@pich
Copy link

pich commented Mar 11, 2022

Thanks, @MarkBaker for the blazing-fast reaction. You are the star in a team of superstars.

@eXsio
Copy link
Author

eXsio commented Mar 12, 2022

Thank you for a quick turnaround! Can't wait for the next release with the fix :)

@eXsio eXsio closed this as completed Mar 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants