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.

Sign up for GitHub

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

Jump to bottom

153/maintenance mode #174

Closed
wants to merge 6 commits into from
Closed

153/maintenance mode #174

wants to merge 6 commits into from

Conversation

serundeputy
Copy link
Member

Adding commands:

  • bee state-get <STATE>
  • bee state-set <STATE> <VALUE>
  • bee maintenance-mode <VALUE>
➜  backdrop git:(153/maintenance-mode) ✗ lando bee help state-get
state-get, sg, sget
Get the value of a Backdrop state.

Arguments:
 state   The name of the state to get. 

Examples:
 bee state-get maintanance_mode   Get the value of the Backdrop state named "maintanance_mode". 

➜  backdrop git:(153/maintenance-mode) ✗ lando bee help state-set
state-set, ss, sset
Set the value of a Backdrop state.

Arguments:
 state   The name of the state to set.  
 value   The value to set the state to. 

Examples:
 bee state-set maintanance_mode 1   Set the value of the Backdrop state named "maintanance_mode" to 1. 

➜  backdrop git:(153/maintenance-mode) ✗ lando bee help maintenance-mode
maintenance-mode, mm
Set the value of maintenance_mode for Backdrop.

Arguments:
 value   The value to set maintenance_mode to. 

Examples:
 bee maintanance-mode 1   Set the value of the Backdrop maintenance_mode to 1.

@serundeputy serundeputy mentioned this pull request Feb 25, 2022
Closed
@serundeputy serundeputy requested a review from a user February 25, 2022 02:34
ghost
ghost suggested changes Feb 25, 2022
View reviewed changes
.lando.yml
Comment on lines -7 to -9
# Although Drush isn't needed/used, the version that Lando includes by default
# has a bug that causes all build steps to fail. We therefore need to update
# Drush to the latest version just to make sure that build steps run properly.
Copy link

Choose a reason for hiding this comment

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

I'd like to keep this comment for now, since it explains why we need to specify the Drush version. It's because Lando defaults to 1.4.0 (https://github.com/lando/backdrop/blob/505b65ebf113887a4d341f252a543c5d8c4ca83d/recipes/backdrop/builder.js#L48) but that version doesn't include this necessary change: backdrop-contrib/backdrop-drush-extension#232 (need a new release for that).

tests/phpunit.xml
@@ -4,6 +4,7 @@
<file>backdrop/BeeCoreTest.php</file>
<file>backdrop/CacheCommandsTest.php</file>
<file>backdrop/ConfigCommandsTest.php</file>
<file>backdrop/StateCommandsTest.php</file>
Copy link

Choose a reason for hiding this comment

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

The rest of the tests are listed alphabetically. Can we move this one down appropriately?

commands/state.bee.inc
<?php
/**
* @file
* Command(s) for getting and setting backdrop states.
Copy link

Choose a reason for hiding this comment

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

Suggested change
* Command(s) for getting and setting backdrop states.
* Command(s) for getting and setting Backdrop states.

commands/state.bee.inc
* Implements hook_bee_command().
*/
function state_bee_command() {
return [
Copy link

Choose a reason for hiding this comment

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

Backdrop hasn't yet changed to using the short array syntax, and neither has Bee. So for consistency with the rest of the code we should use the longer syntax.

If we really want to change to the short syntax, we need a new issue to address this throughout all of Bee.

commands/state.bee.inc
'description' => bt('Get the value of a Backdrop state.'),
'callback' => 'state_get_bee_callback',
'arguments' => [
'state' => bt('The name of the state to get.'),
Copy link

Choose a reason for hiding this comment

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

I don't know much about states, but is it worth making this optional so you can run bee state-get to see a list of all states and their values...? (similar to bee config-get)

Looks like state_initialize() would do it.

Copy link

Choose a reason for hiding this comment

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

Thought about this some more, and this can be a followup issue if there's interest. So happy to leave this as-is for now.

tests/backdrop/StateCommandsTest.php
Comment on lines +15 to +17
$output_all = shell_exec('bee state-get maintenance_mode');
$this->assertStringContainsString('0', $output_all);
$this->assertStringContainsString('maintenance_mode', $output_all);
Copy link

Choose a reason for hiding this comment

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

Suggested change
$output_all = shell_exec('bee state-get maintenance_mode');
$this->assertStringContainsString('0', $output_all);
$this->assertStringContainsString('maintenance_mode', $output_all);
$output = shell_exec('bee state-get maintenance_mode');
$this->assertStringContainsString("The value of the 'maintenance_mode' state is: 0", $output);

tests/backdrop/StateCommandsTest.php
Comment on lines +24 to +29
$output = shell_exec('bee state-get maintenance_mode');
$this->assertStringContainsString('maintenance_mode', $output);
$this->assertStringContainsString('0', $output);

$output = shell_exec('bee state-set maintenance_mode 1');
$this->assertStringContainsString('1', $output);
Copy link

Choose a reason for hiding this comment

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

Suggested change
$output = shell_exec('bee state-get maintenance_mode');
$this->assertStringContainsString('maintenance_mode', $output);
$this->assertStringContainsString('0', $output);
$output = shell_exec('bee state-set maintenance_mode 1');
$this->assertStringContainsString('1', $output);
// Make sure maintenance_mode is '0'.
$output = shell_exec('bee state-get maintenance_mode');
$this->assertStringContainsString("The value of the 'maintenance_mode' state is: 0", $output);
// Set maintenance_mode to '1'.
$output = shell_exec('bee state-set maintenance_mode 1');
$this->assertStringContainsString("The 'maintenance_mode' state was set to: 1", $output);
// Make sure maintenance_mode is '1'.
$output = shell_exec('bee state-get maintenance_mode');
$this->assertStringContainsString("The value of the 'maintenance_mode' state is: 1", $output);

tests/backdrop/StateCommandsTest.php
$this->assertStringContainsString('1', $output);
}

/**
Copy link

Choose a reason for hiding this comment

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

Suggested change
/**
/**

tests/backdrop/StateCommandsTest.php
Comment on lines +37 to +38
$this->assertStringContainsString('maintenance_mode', $output);
$this->assertStringContainsString('1', $output);
Copy link

Choose a reason for hiding this comment

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

Suggested change
$this->assertStringContainsString('maintenance_mode', $output);
$this->assertStringContainsString('1', $output);
$this->assertStringContainsString("The 'maintenance_mode' state was set to: 1", $output);

tests/backdrop/StateCommandsTest.php
$this->assertStringContainsString('1', $output);

$output = shell_exec('bee state-set maintenance_mode 0');
$this->assertStringContainsString('0', $output);
Copy link

Choose a reason for hiding this comment

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

Suggested change
$this->assertStringContainsString('0', $output);
$this->assertStringContainsString("The 'maintenance_mode' state was set to: 0", $output);

ghost
ghost suggested changes Feb 25, 2022
View reviewed changes
commands/state.bee.inc
Comment on lines +58 to +60
$msg = bt('The value of the Backdrop state named "@state" is "@value".', [
'@state' => $arguments['state'],
'@value' => $value,
Copy link

Choose a reason for hiding this comment

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

Actually I'm now thinking this should be displayed via the return value, rather than a message... See https://github.com/backdrop-contrib/bee/blob/1.x-1.x/API.md#command_bee_callback for details and bee config-get for an example (though I don't mind outputting something like the text above as opposed to just the value itself).

@yorkshire-pudding
Copy link
Collaborator

@serundeputy what you've done here is great and will be a fantastic addition to bee. Thank you. Do you have capacity to address @BWPanda 's feedback? If not, are you happy for me to work on that branch to get it over the line?

@serundeputy
Copy link
Member Author

@yorkshire-pudding yes; please pick up the branch. thanks!

@yorkshire-pudding
Merge pull request #221 from backdrop-contrib/1.x-1.x
efe2c1b 
Update branch with commits from 1.x-1.x
@yorkshire-pudding
Copy link
Collaborator

Superceded by PR #232 which incorporates this work and the feedback.

@yorkshire-pudding yorkshire-pudding deleted the 153/maintenance-mode branch November 7, 2022 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@serundeputy @yorkshire-pudding