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

Added ability to filter enabled or disabled with bin/gpm index #3187

Merged
merged 1 commit into from
Feb 8, 2021

Conversation

jgonyea
Copy link
Contributor

@jgonyea jgonyea commented Jan 30, 2021

Third time's the charm (hopefully)

Adds ability to filter index results based on enabled/ disabled settings.

Copy link
Member

@mahagr mahagr left a comment

Choose a reason for hiding this comment

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

A few changes are required, others are nice to have.

$installed = $this->isPluginInstalled($slug);

$enabled = isset($config[$slug]) && ((bool)$config[$slug]['enabled'] == true);
$disabled = isset($config[$slug]['enabled']) && ((bool)$config[$slug]['enabled'] === false);
Copy link
Member

Choose a reason for hiding this comment

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

Umm, aren't these two lines the same?

Here's better code:

return (bool)($config[$slug]['enabled'] ?? false);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there are technically 3 states:

  1. config exists and is set to enabled
  2. config exists and is set to disabled
  3. config doesn't exist (grav treats this as disabled)

I was attempting to take into consideration the tricky difference between #2 and #3 cases.

public function isPluginEnabled($slug)
{
$result = null;
$grav = new Grav();
Copy link
Member

Choose a reason for hiding this comment

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

This won't work. Use $grav = Grav::instance();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix this.

*/
public function isThemeEnabled($slug)
{
$grav = new Grav();
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, use instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above...I'll fix this.

$current_theme = $grav::instance()['config']['system']['pages']['theme'];
$theme_installed = $this->isThemeInstalled($slug);

if ($current_theme == $slug) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd do the same as with plugins. I would also move check for installed outside of this (and plugin) method and check it elsewhere..

@@ -138,7 +150,8 @@ protected function serve(): int
'Name' => '<cyan>' . Utils::truncate($package->name, 20, false, ' ', '...') . '</cyan> ',
'Slug' => $slug,
'Version'=> $this->version($package),
'Installed' => $this->installed($package)
'Installed' => $this->installed($package),
'Enabled' => $this->enabled($package),
Copy link
Member

@mahagr mahagr Feb 1, 2021

Choose a reason for hiding this comment

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

and have something like: 'Enabled' => $this->installed($package) ? $this->enabled($package) : null, here.

Copy link
Member

@mahagr mahagr Feb 2, 2021

Choose a reason for hiding this comment

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

@jgonyea If you check here, this change would check all your 3 cases: true, false and null.

@mahagr mahagr merged commit 7964028 into getgrav:develop Feb 8, 2021
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.

2 participants