Skip to content

[9.x] Fix EventFake::assertListening() for asserting string-based observer listeners#42289

Merged
taylorotwell merged 3 commits into
laravel:9.xfrom
simonhamp:fix/eventfake-model-observers
May 9, 2022
Merged

[9.x] Fix EventFake::assertListening() for asserting string-based observer listeners#42289
taylorotwell merged 3 commits into
laravel:9.xfrom
simonhamp:fix/eventfake-model-observers

Conversation

@simonhamp
Copy link
Copy Markdown
Contributor

@simonhamp simonhamp commented May 6, 2022

Follow-up to #42193

Widening the matching mechanism for string-based assertions (from Str::endsWith('@handle') to Str::contains('@')) allowed the underlying logic to unexpectedly handle Observer event assertions, transforming the $actualListener from a string to an array without also transforming the $expectedListener from a string to an array (where the assertion is a string).

(NB: This wasn't breaking any production code, only some tests that used string-based event listener assertion for observers instead of array-based assertion)

This was missed due to a lack of testing around Observer listener assertion in the EventFake test suite. So this PR contains new tests to cover this scenario.

It addresses the issue by explicitly parsing a string-based $expectedListener assertion when the $actualListener is also a string (now always converted to an array as of #42193), so that the $expectedListener correctly matches the parsed $actualListener.

// $actualListener         $expectedListener
Model::observe(new Observer)  'Observer@method'

// Before fix
['Observer', 'method'] === 'Observer@method'
// => false, failing test

// After fix
['Observer', 'method'] === ['Observer', 'method']
// true, pass

simonhamp added 3 commits May 6, 2022 20:19
The string-based assertion doesn't work. The array-based one does
# Conflicts:
#	src/Illuminate/Support/Testing/Fakes/EventFake.php
@taylorotwell taylorotwell merged commit eb9c371 into laravel:9.x May 9, 2022
@simonhamp simonhamp deleted the fix/eventfake-model-observers branch May 11, 2022 21:43
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.

2 participants