Skip to content

gpm: Plugins containing "theme" should not be installed as themes#2913

Open
drzraf wants to merge 1 commit into
getgrav:developfrom
drzraf:patch-3
Open

gpm: Plugins containing "theme" should not be installed as themes#2913
drzraf wants to merge 1 commit into
getgrav:developfrom
drzraf:patch-3

Conversation

@drzraf

@drzraf drzraf commented May 13, 2020

Copy link
Copy Markdown

Sommerregen/grav-plugin-themer#6
./bin/gpm -vvv direct-install https://github.com/Sommerregen/grav-plugin-themer/archive/v1.1.0.zip

Up to you to evaluate the pattern themes must match, but I felt that theme- would be reasonable... until all this could be replaced by composer (or a composer plugin like symfony flex).

@drzraf drzraf changed the title Plugins containing "theme" should not be treated as themes gpm: Plugins containing "theme" should not be installed as themes May 13, 2020
@mahagr mahagr requested a review from rhukster July 24, 2020 08:52
@mahagr

mahagr commented Jul 24, 2020

Copy link
Copy Markdown
Member

@rhukster @w00fz What do you think? I think it might be better to try to find word "plugin" first, right now the code looks for "theme" and only if it's not found, checks for "plugin" in the name.

@mahagr mahagr requested a review from w00fz July 24, 2020 08:54
@drzraf

drzraf commented Jul 24, 2020

Copy link
Copy Markdown
Author

(One more thing that would be resolved by #2397)

@mahagr

mahagr commented Jul 27, 2020

Copy link
Copy Markdown
Member

What do you think of changing the order of the tests (too)?

@drzraf

drzraf commented Jul 27, 2020

Copy link
Copy Markdown
Author

Will work for me in that specific case. Another possibility is to tighten the match to \btheme\b.

@w00fz

w00fz commented Jul 27, 2020

Copy link
Copy Markdown
Member

I don’t think the order should matter. Matching ^grav-theme.+, ^grav-plugin.+ and ^grav-skeleton.+ should be sufficient.
Where do you see this check happening @mahagr, is it in the internal repo generator? I was pretty sure to have a dash in there but might be wrong.

@mahagr

mahagr commented Jul 27, 2020

Copy link
Copy Markdown
Member

@w00fz The check is in here:

public static function getPackageType($source)
{
$plugin_regex = '/^class\\s{1,}[a-zA-Z0-9]{1,}\\s{1,}extends.+Plugin/m';
$theme_regex = '/^class\\s{1,}[a-zA-Z0-9]{1,}\\s{1,}extends.+Theme/m';
if (
file_exists($source . 'system/defines.php') &&
file_exists($source . 'system/config/system.yaml')
) {
return 'grav';
}
// must have a blueprint
if (!file_exists($source . 'blueprints.yaml')) {
return false;
}
// either theme or plugin
$name = basename($source);
if (Utils::contains($name, 'theme-')) {
return 'theme';
}
if (Utils::contains($name, 'plugin')) {
return 'plugin';
}
foreach (glob($source . '*.php') as $filename) {
$contents = file_get_contents($filename);
if (preg_match($theme_regex, $contents)) {
return 'theme';
}
if (preg_match($plugin_regex, $contents)) {
return 'plugin';
}
}
// Assume it's a theme
return 'theme';
}

It doesn't check anything else but the folder name of zip file if it matches.

@w00fz

w00fz commented Jul 27, 2020

Copy link
Copy Markdown
Member

Actually the package type checks the php class. If it's extending Plugin it's a plugin, if it's extending Theme is a theme..

@w00fz

w00fz commented Jul 27, 2020

Copy link
Copy Markdown
Member

I'm ok with your change @drzraf but could you do the same thing for the plugin check down below?

@mahagr

mahagr commented Jul 28, 2020

Copy link
Copy Markdown
Member

@w00fz Actually it does not. The check for folder name comes first and the rest of the checks are never made.

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