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

[5.8] Fixed circular dependency - fix #28165 #28164

Merged
merged 3 commits into from Apr 11, 2019

Conversation

Projects
None yet
4 participants
@kauanslr
Copy link
Contributor

commented Apr 10, 2019

This PR fix the issue opened by me (#28165).

Basically, this code override the QueueManager::__call magic method when the Queue is mocked to prevend a circular dependency when calling a undefined method in the mock.

To prevent any break of existing feature i've made a simple test.

kauanslr added some commits Apr 10, 2019

@driesvints driesvints changed the title Fixed circular dependency - fix #28163 [5.8Fixed circular dependency - fix #28163 Apr 10, 2019

@driesvints driesvints changed the title [5.8Fixed circular dependency - fix #28163 [5.8] Fixed circular dependency - fix #28163 Apr 10, 2019

@kauanslr kauanslr changed the title [5.8] Fixed circular dependency - fix #28163 [5.8] Fixed circular dependency - fix #28165 Apr 10, 2019

@taylorotwell

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

Tests fail?

@kauanslr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

Tests fail?

Yes, i'm looking this.

Two memcached tests failed with PHP7.1, i'm trying to reproduce locally.

@kauanslr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

Done.

But the Travis is crazy.

@taylorotwell taylorotwell merged commit 68981ba into laravel:5.8 Apr 11, 2019

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@antoniopaisfernandes

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

I've got a failing test when updating to 5.8.12

There was 1 error:

1) Tests\Feature\Claims\ArrivingAtTheWarehouseTest::dri_cores_are_split_by_UK_and_PL
BadMethodCallException: Call to undefined method Illuminate\Support\Testing\Fakes\QueueFake::assertPushedTimes()
@chrismeh

This comment has been minimized.

Copy link

commented Apr 17, 2019

assertPushedTimes is protected. Due to this fix, calls to assertPushedTimes will always hit __call and throw a BadMethodCallException. We could probably just go ahead and make assertPushedTimes public aswell?

@kauanslr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

We'll, looks like there many protected asserts. As @chrismeh mentioned, we cold just make these methods public.
I'll proceed with an PR with this resolution and create some tests to this problem too.

@kauanslr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

I've got a failing test when updating to 5.8.12

There was 1 error:

1) Tests\Feature\Claims\ArrivingAtTheWarehouseTest::dri_cores_are_split_by_UK_and_PL
BadMethodCallException: Call to undefined method Illuminate\Support\Testing\Fakes\QueueFake::assertPushedTimes()

Maybe, a alternative is to use assertPushed and pass times in callback parameter.

@antoniopaisfernandes

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

@kauanslr I've, since, done that.

Queue::assertPushed(PrintAndEmail::class, 2);

I just viewed assertPushedTimes as a more verbose way of testing what I intended.

Thanks anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.