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.

Sign up for GitHub

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

Exclude heading row when limiting imported rows #4217

Open
wants to merge 3 commits into
base: 3.1
Choose a base branch
from
+77 −1
Open

Exclude heading row when limiting imported rows #4217

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fb49cdf
Exclude heading row when limiting imported rows
jivanf Oct 13, 2024
60d0143
Compare array contents instead of length
jivanf Oct 23, 2024
e7e27e7
Add test for multiple limit import with heading row
jivanf Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/Factories/ReaderFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use Maatwebsite\Excel\Concerns\MapsCsvSettings;
use Maatwebsite\Excel\Concerns\WithCustomCsvSettings;
use Maatwebsite\Excel\Concerns\WithHeadingRow;
use Maatwebsite\Excel\Concerns\WithLimit;
use Maatwebsite\Excel\Concerns\WithReadFilter;
use Maatwebsite\Excel\Concerns\WithStartRow;
Expand Down Expand Up @@ -61,8 +62,16 @@ public static function make($import, TemporaryFile $file, string $readerType = n
if ($import instanceof WithReadFilter) {
$reader->setReadFilter($import->readFilter());
} elseif ($import instanceof WithLimit) {
$startRow = 1;

if ($import instanceof WithStartRow) {
$startRow = $import->startRow();
} elseif ($import instanceof WithHeadingRow) {
$startRow = 2;
}

$reader->setReadFilter(new LimitFilter(
$import instanceof WithStartRow ? $import->startRow() : 1,
$startRow,
$import->limit()
));
}
Expand Down
27 changes: 27 additions & 0 deletions tests/Concerns/WithLimitTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use Maatwebsite\Excel\Concerns\Importable;
use Maatwebsite\Excel\Concerns\ToArray;
use Maatwebsite\Excel\Concerns\ToModel;
use Maatwebsite\Excel\Concerns\WithHeadingRow;
use Maatwebsite\Excel\Concerns\WithLimit;
use Maatwebsite\Excel\Concerns\WithStartRow;
use Maatwebsite\Excel\Tests\Data\Stubs\Database\User;
Expand Down Expand Up @@ -104,6 +105,32 @@ public function limit(): int
$import->import('import-users.xlsx');
}

public function test_can_import_with_heading_row()
{
$import = new class implements ToArray, WithLimit, WithHeadingRow
{
use Importable;

/**
* @param array $array
*/
public function array(array $array)
{
Assert::assertCount(1, $array);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we test here the exact contents of the array to be sure that it give the right now?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous version returned an empty array when the limit was 1, which is why I just tested the length of the array, but I see now that the test could return a false positive in the future if, for example, an array with a single null value was returned instead. I'll change that.

Now that I'm thinking about it, a test with a limit of 2 should probably be added as well. When the limit was 2+, all the rows were imported except the last one, which was imported with null values, as the original issue reported. Currently, the test covers that case too, but it'd be safer to have another test for it.

}

/**
* @return int
*/
public function limit(): int
{
return 1;
}
};

$import->import('import-users-with-headings.xlsx');
}

public function test_can_set_limit_bigger_than_row_size()
{
$import = new class implements ToArray, WithLimit
Expand Down