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

fix bug #3865 #3873

Merged
merged 3 commits into from
Feb 16, 2023
Merged

fix bug #3865 #3873

merged 3 commits into from
Feb 16, 2023

Conversation

chickgit
Copy link
Contributor

@chickgit chickgit commented Feb 15, 2023

1️⃣ Why should it be added? What are the benefits of this change?
This PR fix bug at #3865
2️⃣ Does it contain multiple, unrelated changes? Please separate the PRs out.
No
3️⃣ Does it include tests, if possible?
No
4️⃣ Any drawbacks? Possible breaking changes?
No
5️⃣ Mark the following tasks as done:

  • Checked the codebase to ensure that your feature doesn't already exist.
  • Take note of the contributing guidelines.
  • Checked the pull requests to ensure that another person hasn't already submitted a fix.
  • Added tests to ensure against regression.
  • Updated the changelog

@patrickbrouwers
Copy link
Member

Can you please explain the change. Anyway we can add a test for this? Also can you please update the changelog?

@chickgit
Copy link
Contributor Author

In this bug (#3865) the inserted array is in 'A31' instead at 'A24' after heading in 'A23'. This is export example of bug (before fix.xlsx)

After tracing the issue, i got the bug appear in here:

$startCell = CellHelper::getColumnFromCoordinate($startCell) . ($this->worksheet->getHighestRow() + 1);

In PhpOffice\PhpSpreadsheet\Worksheet\Worksheet::getHighestRow is returning Highest row number

We know when exporting using image and array, Laravel-excel write image first then write array after that. because of that getHighestRow() returning '30' instead '23' the startCell() in (#3865)

So in this commit(ec6e21f) i moved the process writing image after writing array instead before writing array. This is example export after fix (after fix.xlsx)

@chickgit
Copy link
Contributor Author

I just using test from laravel-excel

@patrickbrouwers patrickbrouwers merged commit b79b0d4 into SpartnerNL:3.1 Feb 16, 2023
@aaronhuisinga
Copy link
Contributor

aaronhuisinga commented Mar 9, 2023

@patrickbrouwers @chickgit this introduced a regression that has broken a few of our exports.

It appears that this new logic while using headers and a drawing messes things up. Here is a simplified version of one of the exports where we are seeing unintended changes from this PR:

<?php

namespace App\Exports;

use App\Models\Student;
use Illuminate\Contracts\Queue\ShouldQueue;
use Maatwebsite\Excel\Concerns\Exportable;
use Maatwebsite\Excel\Concerns\FromQuery;
use Maatwebsite\Excel\Concerns\ShouldAutoSize;
use Maatwebsite\Excel\Concerns\WithDrawings;
use Maatwebsite\Excel\Concerns\WithHeadings;
use Maatwebsite\Excel\Concerns\WithMapping;
use PhpOffice\PhpSpreadsheet\Worksheet\Drawing;

class StudentProgressExport implements FromQuery, ShouldAutoSize, ShouldQueue, WithDrawings, WithHeadings, WithMapping
{
    use Exportable;

    public int $student;
    public string $logo;

    public function __construct($student, $logo)
    {
        $this->student = $student;
        $this->logo = $logo;
    }

    /**
     * Build the query for export.
     *
     * @return \Illuminate\Database\Eloquent\Builder
     */
    public function query()
    {
        return Student::active();
    }

    /**
     * Map the Student fields for export.
     *
     * @param Student $student
     *
     * @return array
     */
    public function map($student): array
    {
        return [
            $student->Name,
            $student->Email,
            $student->Completed1 ? '✓' : '',
            $student->Completed2 ? '✓' : '',
            $student->CompletedAt,
        ];
    }

    /**
     * Create an array of fields to be included in the export.
     *
     * @return array
     */
    public function headings(): array
    {
        return [
            [
                '',
            ],
            [
                'Name',
                'Email',
                'Test 1',
                'Test 2',
                'Completion Date',
            ]
        ];
    }

    public function drawings()
    {
        $image_contents = file_get_contents($this->logo);
        $temp_image = tempnam(sys_get_temp_dir(), 'logo-');
        file_put_contents($temp_image, $image_contents);

        $drawing = new Drawing();
        $drawing->setName('Logo');
        $drawing->setDescription(__('general.logo'));
        $drawing->setPath($temp_image);
        $drawing->setHeight(90);
        $drawing->setCoordinates('A1');

        return $drawing;
    }
}

What this did pre-change was add an image to cell A1, leave the rest of the first row blank, and add footers in row 2. After that, student data was printed.

Before PR merged (correct export):
working

After PR merged (incorrect export):
broken

With the drawing being inserted at the end, it appears to break things pretty badly. I don't think we're doing anything we aren't supposed to be in our export logic, and so it seems to me this is an unintended change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants