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

[lkraav]

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]

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]

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]

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]

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]

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]

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 https://github.com/cedaro/satispress/blob/af196ed5cab5cb2f2c51487788a8ca7df8d06bdd/src/Transformer/ComposerPackageTransformer.php#L64 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]

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]

@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