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

0.4.1: Repository/Installed{Plugins,Themes}::all() overwrite keys when plugins and themes slugs match #109

Closed
lkraav opened this issue Sep 6, 2019 · 8 comments · May be fixed by #130
Closed

Comments

@lkraav
Copy link

lkraav commented Sep 6, 2019

This is probably an edge case, but in the case you have both a plugin and a theme directories named for example my-company, matching ::all() array key will get overwritten, and I am not able to enable the plugin for Satispress. Crash signature below:

Error: Call to a member function is_installed() on null
#11 public_html/institute/wp-content/plugins/satispress/src/Provider/PackageArchiver.php(217): archive_package
#10 public_html/institute/wp-content/plugins/satispress/src/Provider/PackageArchiver.php(201): archive_packages
#9 public_html/institute/wp-content/plugins/satispress/src/Provider/PackageArchiver.php(131): archive_on_option_update
#8 public_html/institute/wp-includes/class-wp-hook.php(286): apply_filters
#7 public_html/institute/wp-includes/class-wp-hook.php(310): do_action
#6 public_html/institute/wp-includes/plugin.php(465): do_action
#5 public_html/institute/wp-includes/option.php(411): update_option
#4 public_html/institute/wp-content/plugins/satispress/src/Screen/ManagePlugins.php(89): ajax_toggle_plugin_status
#3 public_html/institute/wp-includes/class-wp-hook.php(286): apply_filters
#2 public_html/institute/wp-includes/class-wp-hook.php(310): do_action
#1 public_html/institute/wp-includes/plugin.php(465): do_action
#0 admin-ajax.php(173): null

I think prefixing all keys with object type might be the simplest solution, so we'd get theme-my-company and plugin-my-company, but I don't know SP internals yet to understand where else these keys are being used.

Your thoughts?

@lkraav
Copy link
Author

lkraav commented Sep 6, 2019

Itmw, I'm able to work around this by forcing SP to return an empty $items array for themes at https://github.com/cedaro/satispress/blob/v0.4.1/src/Repository/InstalledThemes.php#L53 commenting out foreach loop.

@bradyvercher
Copy link
Member

I was wondering if that would ever become an issue. I don't think there's a way to differentiate between those right now and it might be difficult since the slug is the only thing used throughout SatisPress to identify packages. Ideally they should be unique. I'm guessing this might be one reason WordPress Packagist uses the wpackagist-plugin and wpackagist-theme vendor names.

If you have control over the theme and plugin, I'd recommend giving them unique names.

Beyond that, it sounds like you might be encountering an error that we could potentially prevent? Did you add both packages to SatisPress? Or do you get the fatal if only one of them is added?

@lkraav
Copy link
Author

lkraav commented Sep 20, 2019

I did not enable any themes for Satispress at all, so it looks like just adding one of them (in this case, the plugin) is enough to confuse the system.

Order of operation might also matter. If themes are checked first, they will always win the array key race.

@bradyvercher
Copy link
Member

I wasn't totally clear what was going on here, but after getting a chance to dig in, I see which keys you were referring to. Those aren't really used anywhere, so prefixing (or removing them) should be fine.

It still wouldn't be possible to add two packages with the same name, but updating those keys will prevent that error.

@lkraav
Copy link
Author

lkraav commented Sep 25, 2019

It still wouldn't be possible to add two packages with the same name, but updating those keys will prevent that error.

Sounds good (enough) to me.

@lkraav
Copy link
Author

lkraav commented Jun 12, 2020

It still wouldn't be possible to add two packages with the same name, but updating those keys will prevent that error.

To try to get this working via dynamic vendor filtering, do you think

$vendor = apply_filters( 'satispress_vendor', 'satispress' );
could also send $package as a filter arg, so we could have vendor-theme, vendor-plugin depending on the package?

For example, doing a super simple monkey-patch like $vendor = apply_filters( 'satispress_vendor', 'satispress' ) . '-' . $package->get_type(); seems to work exactly as I'd want.

@bradyvercher
Copy link
Member

Yeah, that can be added as an arg to that filter.

I'd have to do some testing, but I don't think it'll totally do what you're wanting because the download route doesn't receive any information to disambiguate the request by package type. All it receives is the slug and version.

At the very least, I think the release URL would probably need to have a query parameter specifying the package type and the download route would need to use that to look up the package.

@saas786
Copy link

saas786 commented Jun 21, 2020

@bradyvercher

Thanks for pointers, I have compiled the above pull request. I have tested it on my local website with composer / direct download from dashboard, it seems to work fine for me.

Will appreciate if you can give it a try and provide feedback if any.

As its a critical and long standing issue for us, would love to wrap it ASAP.

Thanks

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 a pull request may close this issue.

3 participants