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

Added BeforeChunk and AfterChunk events #3634

Closed
wants to merge 11 commits into from
Closed

Conversation

colq2
Copy link

@colq2 colq2 commented May 27, 2022

1️⃣ Why should it be added? What are the benefits of this change?

This PR adds two new events: BeforeChunk and AfterChunk.
These are triggered when exporting in chunks. With this it is possible to

  • showing the progress to the user
  • estimate time for an export

Related issue: #3606

2️⃣ Does it contain multiple, unrelated changes? Please separate the PRs out.

No.

3️⃣ Does it include tests, if possible?

It includes tests, but I don't know if you are quite happy with it. The test should cover, that the events are called exactly for the number of chunks. For this purpose, groups are created in the callback and counted at the end. Unfortunately I couldn't put in a callback because it can't be serialized.

If you have a better idea, let me know. I'm happy to change it.

4️⃣ Any drawbacks? Possible breaking changes?

No breaking changes.

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

6️⃣ Thanks for contributing! 🙌
🙌

use Maatwebsite\Excel\Tests\Data\Stubs\Database\User;
use Maatwebsite\Excel\Tests\Data\Stubs\QueuedExportWithChunkEvents;

class QueuedExportWithChunkEventsTest extends TestCase
Copy link
Member

Choose a reason for hiding this comment

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

Somehow this test seems to be failing in GH Actions, can you have a look?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed it. Sorry, didn't saw the GH actions.

@patrickbrouwers
Copy link
Member

Can you already draft up a PR to the docs as well? Thanks!

@colq2
Copy link
Author

colq2 commented Jun 12, 2022

Sure, added it!
While creating the PR I hove noticed, that the new events couldn't be registered with the RegistersEventListeners concern. So I added another test and relevant code for it.

Another point I noticed is, that all other events have some sort of context / payload e.g Writer or Sheet. It could be helpful to also add a context into BeforeChunk and AfterChunk like the Writer or the AppendQueryToSheet.
What do you think?

@stale stale bot added the stale label Aug 11, 2022
@stale
Copy link

stale bot commented Aug 12, 2022

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.

@stale stale bot closed this Aug 12, 2022
@Djoul
Copy link

Djoul commented Feb 8, 2023

Hi, any reason to not merge this PR? it's a nice feature to have for all the people that want to handle the export progress. Thanks

@HristoAntov
Copy link

+1 on merging the PR. I see no other reason for closing than inactivity.
These events are really useful for tracking current status of big exports and currenly I've found no other way to do it. Thanks

@sebestenyb
Copy link
Contributor

Can we reopen this? It is a very much missed feature.

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

Successfully merging this pull request may close these issues.

None yet

5 participants