-
-
Notifications
You must be signed in to change notification settings - Fork 51
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
Comments
Itmw, I'm able to work around this by forcing SP to return an empty |
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 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? |
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. |
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. |
Sounds good (enough) to me. |
To try to get this working via dynamic vendor filtering, do you think
$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 |
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. |
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 |
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:I think prefixing all keys with object type might be the simplest solution, so we'd get
theme-my-company
andplugin-my-company
, but I don't know SP internals yet to understand where else these keys are being used.Your thoughts?
The text was updated successfully, but these errors were encountered: