Skip to content

Fix: AfterChunk event raised after sheet disconnect - #4385

Open
zigzagdev wants to merge 2 commits into
SpartnerNL:4.xfrom
zigzagdev:fix/after-chunk-method-timing
Open

Fix: AfterChunk event raised after sheet disconnect#4385
zigzagdev wants to merge 2 commits into
SpartnerNL:4.xfrom
zigzagdev:fix/after-chunk-method-timing

Conversation

@zigzagdev

Copy link
Copy Markdown

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

AfterChunk was fired after Sheet::disconnect() in ReadChunk::handle().
disconnect() calls unset($this->worksheet), so any event handler that accesses $event->getSheet()->getDelegate() would encounter an undefined property and receive null instead of the expected Worksheet instance.
Moving raise(AfterChunk) before disconnect() ensures the worksheet is still accessible inside event handlers, which is the naturally expected behavior.

2️⃣Does it contain multiple, unrelated changes?

No.
This PR contains a single focused fix: reordering raise(AfterChunk) and disconnect() in ReadChunk::handle().

3️⃣Does it include tests, if possible?

Yes.
test_after_chunk_event_sheet_delegate_is_accessible has been added to WithEventsTest, asserting that $event->getSheet()->getDelegate() returns a Worksheet instance within an AfterChunk handler.

4️⃣Any drawbacks? Possible breaking changes?

No breaking changes.
The fix only corrects the order of two operations. Event handlers that do not access getDelegate() are unaffected.

5️⃣Tasks

  • 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.

Test Results

screen capture 2026-06-06 0 21 49

@patrickbrouwers

Copy link
Copy Markdown
Member

can you retarget this to 4.x

@patrickbrouwers
patrickbrouwers changed the base branch from 3.1 to 4.x July 9, 2026 08:02
@zigzagdev
zigzagdev changed the base branch from 4.x to 3.1 July 13, 2026 02:56
@zigzagdev
zigzagdev changed the base branch from 3.1 to 4.x July 13, 2026 02:56
@zigzagdev
zigzagdev force-pushed the fix/after-chunk-method-timing branch from a7d43d8 to f3f520c Compare July 14, 2026 11:24
@zigzagdev
zigzagdev force-pushed the fix/after-chunk-method-timing branch from 1224015 to dc5012e Compare July 14, 2026 12:51
@zigzagdev

Copy link
Copy Markdown
Author

@patrickbrouwers

can you retarget this to 4.x

I changed target branch. So please check my PR.

Comment thread src/Jobs/ReadChunk.php
Comment on lines +163 to 167
$sheet->raise(new AfterChunk($sheet, $this->import, $this->startRow));

$sheet->disconnect();

$this->cleanUpTempFile();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

NIT: raise can throw so just in case it does it could be best to make sure the disconnect and cleanup still happens.

Suggested change
$sheet->raise(new AfterChunk($sheet, $this->import, $this->startRow));
$sheet->disconnect();
$this->cleanUpTempFile();
try {
$sheet->raise(new AfterChunk($sheet, $this->import, $this->startRow));
} finally {
$sheet->disconnect();
$this->cleanUpTempFile();
}

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.

3 participants