[Bug]: Worksheet names are interpreted as index

[Ligniperdus]

Is the bug applicable and reproducable to the latest version of the package and hasn't it been reported before?

• [X] Yes, it's still reproducable

What version of Laravel Excel are you using?

3.1.48

What version of Laravel are you using?

9.52.5

What version of PHP are you using?

8.1.12

Describe your issue

Worksheet names are interpreted as index if the name is a number

How can the issue be reproduced?

Define the worksheet name explizit as a string.

Here is a code example inspired by the documentation:

namespace App\Imports;

use Maatwebsite\Excel\Concerns\WithMultipleSheets;

class UsersImport implements WithMultipleSheets 
{
    public function sheets(): array
    {
        $sheetName = "123";
        return [
            $sheetName => new FirstSheetImport(),
        ];
    }
}

Could results in a SheetNotFoundException if the number of sheets is below the actual number of sheets: Maatwebsite\Excel\Exceptions\SheetNotFoundException: Your requested sheet index: 123 is out of bounds. ...

Or worse, the wrong sheet could be taken

What should be the expected behaviour?

The variable should be interpreted as a string and not as an integer.

Suggestions to what is wrong

File: src/Sheet.php Line: 106

The function is_numeric finds whether a variable is a number or a numeric string

Possible solution:

File: src/Sheet.php Line: 106

if (is_numeric($index)) -> if (is_int($index))

[patrickbrouwers]

Can you PR this

[Sanghwan-TIO]

Thanks Ligniperdus

The same issue happened on my project. Maatwebsite\Excel\Exceptions\SheetNotFoundException: Your requested sheet index: 20230410 is out of bounds. The actual number of sheets is 1.

/**
 * @param  Spreadsheet  $spreadsheet
 * @param  string|int  $index
 * @return Sheet
 *
 * @throws \PhpOffice\PhpSpreadsheet\Exception
 * @throws SheetNotFoundException
 */
public static function make(Spreadsheet $spreadsheet, $index)
{
    if (is_numeric($index)) {  
        return self::byIndex($spreadsheet, $index);
    }
    return self::byName($spreadsheet, $index);
}

"is_numeric($index)" should be "is_int($index)"

[patrickbrouwers]

As said before, feel free to PR it

[Ligniperdus]

Actually wanted to finish the PR tonight. However, when testing it became clear to me that there is no simple solution here.

The problem is that PHP automatically covert numeric strings to integers when they are used as array keys. So if you have to distinguish between an index and a name for a worksheet name you can't keep it as a key like before.

I have written a test for this behavior in the WithMultipleSheetsTest.php:

/**
 * @test
 */
public function can_import_multiple_sheets_by_numeric_sheet_name()
{
    $import = new class implements WithMultipleSheets
    {
        use Importable;

        public array $sheets = [];

        public function __construct()
        {
            $this->sheets = [
                "987654321" => new class implements ToArray
                {
                    public bool $called = false;

                    public function array(array $array): void
                    {
                        $this->called = true;
                        Assert::assertEquals([
                            ['1.A1', '1.B1'],
                            ['1.A2', '1.B2'],
                        ], $array);
                    }
                },
            ];
        }

        public function sheets(): array
        {
            return $this->sheets;
        }
    };

    $import->import('import-multiple-sheets.xlsx');

    foreach ($import->sheets as $sheet) {
        $this->assertTrue($sheet->called);
    }
}

I can't think of a good way to continue to store the worksheet name directly as a key. This would mean in consequence that a completely new implementation is necessary in which the worksheet sheet name is stored e.g. also as value in the array or a new object is created. In any case, a considerable amount of code would have to be adapted and the documentation updated. I think I'd rather hand this over to someone more involved in this project.

But maybe there is a simpler solution that I don't see right now. Maybe a special prefix or suffix, but that would not be the most elegant solution.

[stale[bot]]

This bug report has been automatically closed because it has not had recent activity. If this is still an active bug, please comment to reopen. Thank you for your contributions.