Skip to content

Commit adde74b

Browse files
Fix DIMENSIONS record to use 0-based indices for both rows AND columns
The initial fix only converted column indices to 0-based, but overlooked that row indices also need the same treatment per BIFF8 specification. Changes: - Convert firstRowIndex from 1-based to 0-based (subtract 1) - Convert lastRowIndex from 1-based to 0-based (subtract 1) - Update row capping logic for 65536 limit - Fix test assertions to expect rwMic=0 and rwMac=5 (not 1 and 6) - Enhanced documentation to clarify all DIMENSIONS indices are 0-based This now matches the behavior observed in Excel-generated XLS files, where a file with 3 rows × 5 columns shows: rwMic=0, rwMac=3, colMic=0, colMac=5 All tests pass (53 tests, 244 assertions).
1 parent f2ea8b6 commit adde74b

File tree

2 files changed

+24
-19
lines changed

2 files changed

+24
-19
lines changed

src/PhpSpreadsheet/Writer/Xls/Worksheet.php

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -211,10 +211,12 @@ public function __construct(int &$str_total, int &$str_unique, array &$str_table
211211
$maxC = $this->phpSheet->getHighestColumn();
212212

213213
// Determine lowest and highest column and row
214-
$this->firstRowIndex = $minR;
215-
$this->lastRowIndex = ($maxR > 65535) ? 65535 : $maxR;
214+
// BIFF8 DIMENSIONS record requires 0-based indices for both rows and columns
215+
// Row methods return 1-based values (Excel UI), so subtract 1 to convert to 0-based
216+
$this->firstRowIndex = $minR - 1;
217+
$this->lastRowIndex = ($maxR > 65536) ? 65535 : ($maxR - 1);
216218

217-
// BIFF8 requires 0-based column indices, but columnIndexFromString() returns 1-based
219+
// Column methods return 1-based values (columnIndexFromString('A') = 1), so subtract 1
218220
$this->firstColumnIndex = Coordinate::columnIndexFromString($minC) - 1;
219221
$this->lastColumnIndex = min(255, Coordinate::columnIndexFromString($maxC) - 1);
220222
$this->writerWorkbook = $writerWorkbook;

tests/PhpSpreadsheetTests/Writer/Xls/DimensionsRecordTest.php

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -11,19 +11,22 @@
1111
class DimensionsRecordTest extends TestCase
1212
{
1313
/**
14-
* Test that DIMENSIONS record uses 0-based column indices.
14+
* Test that DIMENSIONS record uses 0-based indices for both rows and columns.
1515
*
1616
* This test verifies that the BIFF8 DIMENSIONS record correctly uses 0-based
17-
* column indices. Prior to the fix, columnIndexFromString() returned 1-based
18-
* indices which were written directly to the DIMENSIONS record, causing issues
19-
* with old XLS parsers that expect 0-based indices per the BIFF8 specification.
17+
* indices for both rows and columns. Prior to the fix, 1-based values were
18+
* written directly to the DIMENSIONS record, causing issues with old XLS parsers
19+
* that expect 0-based indices per the BIFF8 specification.
2020
*
2121
* The DIMENSIONS record structure (BIFF8):
22-
* - Offset 0-3: Index to first used row
23-
* - Offset 4-7: Index to last used row + 1
22+
* - Offset 0-3: Index to first used row (0-based)
23+
* - Offset 4-7: Index to last used row + 1 (0-based)
2424
* - Offset 8-9: Index to first used column (0-based)
2525
* - Offset 10-11: Index to last used column + 1 (0-based)
2626
* - Offset 12-13: Not used
27+
*
28+
* Note: All indices in the DIMENSIONS record are 0-based, meaning Excel row 1
29+
* is stored as 0, row 5 as 4, column A as 0, column D as 3, etc.
2730
*/
2831
public function testDimensionsRecordUsesZeroBasedColumnIndices(): void
2932
{
@@ -62,22 +65,22 @@ public function testDimensionsRecordUsesZeroBasedColumnIndices(): void
6265
$data = unpack('VrwMic/VrwMac/vcolMic/vcolMac/vreserved', $dimensionsData);
6366
self::assertIsArray($data, 'Failed to unpack DIMENSIONS record');
6467

65-
// Verify the values are correct (0-based for columns, 1-based for rows)
66-
// First used row is 1
67-
self::assertSame(1, $data['rwMic'], 'First row should be 1');
68+
// Verify the values are correct (0-based for both rows and columns)
69+
// First used row is 1 (Excel UI), which is 0 in 0-based indexing
70+
self::assertSame(0, $data['rwMic'], 'First row should be 0 (0-based)');
6871

69-
// Last used row is 5, so rwMac should be 6 (last row + 1)
70-
self::assertSame(6, $data['rwMac'], 'Last row + 1 should be 6');
72+
// Last used row is 5 (Excel UI), which is 4 in 0-based, so rwMac should be 5 (4 + 1)
73+
self::assertSame(5, $data['rwMac'], 'Last row + 1 should be 5 (0-based row 4 + 1)');
7174

72-
// First column is A (0-based: 0)
75+
// First column is A (Excel UI), which is 0 in 0-based indexing
7376
// BEFORE FIX: This would be 1 (because columnIndexFromString('A') returns 1)
74-
// AFTER FIX: This should be 0 (because we subtract 1)
77+
// AFTER FIX: This is 0 (because we subtract 1)
7578
self::assertSame(0, $data['colMic'], 'First column should be 0 (0-based for column A)');
7679

77-
// Last column is D (0-based index 3), so colMac should be 4 (last used + 1)
80+
// Last column is D (Excel UI), which is 3 in 0-based, so colMac should be 4 (3 + 1)
7881
// BEFORE FIX: This would be 5 (columnIndexFromString('D') = 4, not adjusted to 0-based)
79-
// AFTER FIX: This should be 4 (columnIndexFromString('D') - 1 = 3, + 1 = 4)
80-
self::assertSame(4, $data['colMac'], 'Last column + 1 should be 4 (0-based for column D + 1)');
82+
// AFTER FIX: This is 4 (columnIndexFromString('D') - 1 = 3, + 1 = 4)
83+
self::assertSame(4, $data['colMac'], 'Last column + 1 should be 4 (0-based column 3 + 1)');
8184
}
8285

8386
/**

0 commit comments

Comments
 (0)