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] Add DatabaseRefreshed event to be emitted after database refreshed #34952

Merged
merged 3 commits into from
Oct 27, 2020
Merged

[8.x] Add DatabaseRefreshed event to be emitted after database refreshed #34952

merged 3 commits into from
Oct 27, 2020

Conversation

hotmeteor
Copy link
Contributor

This PR adds a new DatabaseRefreshed event that is emitted after either php artisan migrate:fresh or php artisan migrate:refresh is run.

Being able to listen to this event will allow developers to perform secondary actions to their local environment after refreshing, for example clearing stale or invalid cookies or redefining environment variables related to obsolete data.

@hotmeteor hotmeteor changed the title Add DatabaseRefreshed event to be emitted after refreshed database [8.x] Add DatabaseRefreshed event to be emitted after refreshed database Oct 24, 2020
@hotmeteor hotmeteor changed the title [8.x] Add DatabaseRefreshed event to be emitted after refreshed database [8.x] Add DatabaseRefreshed event to be emitted after database refreshed Oct 24, 2020
@GrahamCampbell
Copy link
Member

This is technically a breaking change, if anyone is extending those commands, since now their calls to the parent constructor will be broken.

@GrahamCampbell
Copy link
Member

Doesn't laravel support injecting dependencies as params to handle? That would be a good way around the constructor changes.

@hotmeteor
Copy link
Contributor Author

@GrahamCampbell Ya that may be an alternative. I was trying to stick to the pattern the other Database events were using.

@GrahamCampbell
Copy link
Member

Right, ok. Don't make any changes. We can wait for Taylor to review this on Monday.

@paras-malhotra
Copy link
Contributor

It would be a breaking change even if the dependencies are injected into handle rather than __construct.

@hotmeteor
Copy link
Contributor Author

@paras-malhotra thats a good point

@taylorotwell
Copy link
Member

taylorotwell commented Oct 26, 2020

To make this not breaking you probably need to service locate the dispatcher out of the container. The container is already available to commands as $this->laravel... $this->laravel[Dispatcher::class]. I don't really worry too much about service location in console commands.

@driesvints
Copy link
Member

The illuminate/events package will need to be added as a dependency in composer.json of the database component.

@hotmeteor
Copy link
Contributor Author

@taylorotwell Updated to use the container.

@hotmeteor
Copy link
Contributor Author

@driesvints Looks like there's a "suggest" in there for that already: https://github.com/illuminate/database/blob/master/composer.json#L41

@driesvints
Copy link
Member

@hotmeteor perfect 👍

@taylorotwell taylorotwell merged commit eb7f854 into laravel:8.x Oct 27, 2020
Comment on lines +62 to +65
$this->laravel[Dispatcher::class]->dispatch(
new DatabaseRefreshed()
);

Copy link
Contributor

@carestad carestad Nov 4, 2020

Choose a reason for hiding this comment

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

Would it be possible to move this so it dispatches before the seeders are being run? Or have a separate event or something for that? It will run at different times now depending on whether you run migrate:fresh --seed or migrate:fresh && db:seed

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.

None yet

6 participants