Skip to content

Conversation

@wimulkeman
Copy link
Contributor

Moving the maintenance mode logic to its own class. This enables users to extend/overwrite the class with their own logic.

Overwriting the logic may be needed if for example the application is running on multiple servers.

Fixes #36474

Moving the maintenance mode logic to its own class. This enables users to extend/overwrite the class with their own logic.

Overwriting the logic may be needed if for example the application is running on multiple servers.

Fixes #36474
@vurpa
Copy link
Contributor

vurpa commented Dec 16, 2021

I think that up/down naming can be a bit confusing, because I think of it the opposite way of how it's currently implemented.

Something like enable/disable or enter/leave feels like more natural verbs to work with.

@wimulkeman
Copy link
Contributor Author

@vurpa The enter/leave names would be a good replacement. I used the up/down names for now to stay in sync with the commands used to start and stop the maintenance mode. If requested by the core developers I would be glad to change it.

* @return void
*/
public function __construct(Application $app)
public function __construct(Application $app, MaintenanceMode $maintenanceMode)
Copy link
Member

Choose a reason for hiding this comment

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

Adding a new dependency here is a breaking change unfortunately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@driesvints You're right. It would for example break implementation if people have extended the class. Should I point the update to the Laravel 9 branch instead?

protected $except = [];

/**
* @var MaintenanceMode
Copy link
Member

Choose a reason for hiding this comment

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

Please use FQN and a short description for DocBlocks

{
try {
if (! is_file(storage_path('framework/down'))) {
if ($maintenanceMode->isDown() === false) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add an isUp method as well.

* @return int
*/
public function handle()
public function handle(MaintenanceMode $maintenanceMode)
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change.

* @return int
*/
public function handle()
public function handle(MaintenanceMode $maintenanceMode)
Copy link
Member

Choose a reason for hiding this comment

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

Another breaking change here.

public function isDownForMaintenance()
{
return file_exists($this->storagePath().'/framework/down');
return app(MaintenanceMode::class)->isDown();
Copy link
Member

@driesvints driesvints Dec 16, 2021

Choose a reason for hiding this comment

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

Suggested change
return app(MaintenanceMode::class)->isDown();
return $this->make(MaintenanceMode::class)->isDown();

@driesvints
Copy link
Member

There's quite a few breaking changes here so I suggest this is best sent to master. Please also take care of the other comments. Thanks

@driesvints driesvints closed this Dec 16, 2021
@wimulkeman wimulkeman deleted the #36474_enable_overwriting_maintenance_methods branch December 16, 2021 08:44
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