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

[8.x] Use the Config facade instead #442

Merged
merged 3 commits into from
Jan 8, 2021
Merged

[8.x] Use the Config facade instead #442

merged 3 commits into from
Jan 8, 2021

Conversation

gocanto
Copy link
Contributor

@gocanto gocanto commented Jan 8, 2021

Description

This PR replaces the use of the config() helper to retrieve the after_commit value when creating the ModelObserver.

The reason why we would want to have this change is, we would be able to mock the configuration object when not extending the application within unit tests.

As these changes stand by now, they break all tests that call/create models using the searchable trait when extending the PHPUnit tests case class [PHPUnit\Framework\TestCase].

Even though it is known we could use these helpers and we should be extending the framework test case class to work on apps tests, they are not resolved if the Laravel app is not present (in the unit context).

Reference: #440

Thank you.

@driesvints driesvints changed the title Use the Config facade instead [8.xUse the Config facade instead Jan 8, 2021
@driesvints driesvints changed the title [8.xUse the Config facade instead [8.x] Use the Config facade instead Jan 8, 2021
@taylorotwell taylorotwell merged commit d978018 into laravel:8.x Jan 8, 2021
@gocanto
Copy link
Contributor Author

gocanto commented Jan 8, 2021

@taylorotwell @driesvints thank you guys. Would we be able to have these changes within the next release? If so, when will it be?

Cheers!

@driesvints
Copy link
Member

Releases are done on Tuesdays.

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