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

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

drzraf
Copy link
Contributor

@drzraf drzraf commented May 13, 2020

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
Copy link
Member

mahagr commented Jul 24, 2020

@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
Copy link
Contributor Author

drzraf commented Jul 24, 2020

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

@mahagr
Copy link
Member

mahagr commented Jul 27, 2020

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

@drzraf
Copy link
Contributor Author

drzraf commented Jul 27, 2020

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

@w00fz
Copy link
Member

w00fz commented Jul 27, 2020

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
Copy link
Member

mahagr commented Jul 27, 2020

@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
Copy link
Member

w00fz commented Jul 27, 2020

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
Copy link
Member

w00fz commented Jul 27, 2020

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

@mahagr
Copy link
Member

mahagr commented Jul 28, 2020

@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