Import events with chunk reading

[sgilberg]

Requirements

Please take note of our contributing guidelines: https://laravel-excel-docs.dev/docs/3.0/getting-started/contributing
Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.

Mark the following tasks as done:

• Checked the codebase to ensure that your feature doesn't already exist.
• Checked the pull requests to ensure that another person hasn't already submitted the feature or fix.
• Adjusted the Documentation. (TBD - see question below)
• Added tests to ensure against regression.

Description of the Change

Adds a single job to the beginning and end of the job chain for imports using the WithChunkReading concern, as discussed in #1944. The jobs fire a BeforeChunkImport and AfterChunkImport event, respectively.

Why Should This Be Added?

I can't speak to all use cases, but for implementations that require WithChunkReading for large imports, we need a way to identify when the chain is complete and perform any related functionality (such as an email notification). Imports that don't use chunk reading already raise a BeforeImport and AfterImport event, but this is not available for the chunked imports yet.

Benefits

Users of the WithChunkReading concern can register listeners for the BeforeChunkImport and AfterChunkImport events to perform any custom functionality before or after a chunk reading import. The event class includes methods that allow you to access the underlying reader class and the import class, similar to the BeforeImport and AfterImport events.

(NOTE: The existing BeforeImport and AfterImport events could not be used, because they include the full Reader class in their payload, which cannot be serialized due to the inclusion of a Closure. The BeforeChunkImport and AfterChunkImport events here use the underlying PhpSpreadsheet class IReader instead.)

Possible Drawbacks

@patrickbrouwers noted in #1944 that there would be a performance impact from two additional jobs in the chain. In testing this in my implementation, I found performance impact to be negligible.

I'd also like to note that I looked into adding the events to the first and last chunk jobs in the chain, rather than as separate jobs, but I found that to be problematic as the ReadChunk job doesn't include the original $import object. Open to suggestions though.

Verification Process

I wrote a test for a new import class that uses both WithChunkReading and WithEvents, modeled after existing event tests, and the full test suite ran smoothly. I also tested this branch in my project implementation, including using the RegistersEventListeners trait, and was able to do the post-import processing I needed using an afterChunkImport() method.

Applicable Issues

#1944 [QUESTION] after import event

Additional notes/questions

I may not have fully understood the intent of all the moving pieces here, and am happy to revise based on others' suggestions. My hope is to at least get the ball moving on a fix for the issue noted above, as this is critical to my implementation.

Also happy to update the docs, but I wasn't sure how thorough a change was needed there. Is it sufficient to add the BeforeChunkImport and AfterChunkImport options to the "Available Events" table in the "Extending" page, or do we think this requires further explanation, or its own section on that page?

[patrickbrouwers]

Hey @sgilberg thanks for the PR!

we need a way to identify when the chain is complete and perform any related functionality (such as an email notification).

This you can already do by chaining a job yourself:

$import->queue('users.xlsx')->chain([
    new NotifyUserOfCompletedImport(request()->user()),
]);

In testing this in my implementation, I found performance impact to be negligible.

If it's just to send a notification, then indeed the performance impact will be very little. I was thinking about situations that you might wish to do stuff with the Reader that would require loading the sheet into memory.

I'll look at the code tomorrow! Thanks for the contribution :)

[sgilberg]

Ah, thanks for the tip! I hadn't realized that I could directly access and add to the chain like that. Just tried it and it works great for my implementation. Perhaps it's worth adding that example to the docs? I'll leave this PR open though in case it's useful for other situations.

[patrickbrouwers]

@sgilberg yes, it's a gap in the documentation. Will add it.

[patrickbrouwers]

@sgilberg updated the docs: https://laravel-excel.maatwebsite.nl/3.1/imports/queued.html

[sgilberg]

@sgilberg updated the docs: https://laravel-excel.maatwebsite.nl/3.1/imports/queued.html

Looks perfect, thank you!

[dalholm]

When will we have this to a new release. im waiting on it :)

[patrickbrouwers]

@dalholm no plan to merge this right now. The chained job suffices in most situations and I'd rather solve the before/after events in a different way; that's on my todo list still.

[dalholm]

@patrickbrouwers Ok i'll understand. 👍

[eoghanobrien]

@patrickbrouwers Any idea when the alternative solution will be available? Is this something that will be solved this month or are we talking far into the future?

[patrickbrouwers]

Can't promise this month, but it's quite high on my list to have a look at.

[eoghanobrien]

@patrickbrouwers Good to hear. Will you updating this thread after you release? How can I stay in the loop?

Great work on this project and thanks for everything you and other contributors add!

[patrickbrouwers]

Yes will update this thread and most likely tweet about it.

[patrickbrouwers]

@sgilberg @eoghanobrien @dalholm Next release (currently 3.1-dev) will have support for Before/AfterImport events for chunk reading. It will also add ImportFailed event that gets raised when any of the chunk jobs fail. Feel free to test it on the 3.1 branch and let me know if it works like excepted.

[eoghanobrien]

@patrickbrouwers Thank you sir, I appreciate your work! I'll check it out early next week.

[patrickbrouwers]

@sgilberg @eoghanobrien @dalholm anybody any feedback?

[eoghanobrien]

@patrickbrouwers Seems to be working great for me, I just added the WithChunkReading and ShouldQueue interfaces back on my Import class. I have the following in my chunkSize method:

public function chunkSize(): int
{
    return 1000;
}

I did have to remove the WithProgressBar interface to get it to work though. Which makes sense since it's on a queue now.

[patrickbrouwers]

@eoghanobrien sounds good! :) And yeah indeed, won't work with that, as it's async now.

[eoghanobrien]

Ah, one snag, it looks like BeforeImport and AfterImport events don't seem to run in the queue. I can understand the AfterImport would be difficult since it's async but I would have expected BeforeImport to run.

[patrickbrouwers]

@eoghanobrien BeforeImport runs outside of the queue, before the process is queued. AfterImport runs in a separate queue job. The both expect the event to be registered on the main import object.

https://github.com/Maatwebsite/Laravel-Excel/blob/3.1/src/ChunkReader.php#L30
https://github.com/Maatwebsite/Laravel-Excel/blob/3.1/src/ChunkReader.php#L61

[sgilberg]

@patrickbrouwers I just tested in my implementation and the AfterImport event ran successfully on a chunked, queued import, thank you! Didn't test BeforeImport, and I'm not accessing $event->reader or anything, but doing basic stuff in a closure works well.

[patrickbrouwers]

Thanks for testing @sgilberg

[eoghanobrien]

Okay, I was able to figure this out, I was referencing the wrong commit in my composer.json. Works as you described. Thanks!

[patrickbrouwers]

Version is released