-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Plugin Dependencies #1674
Plugin Dependencies #1674
Conversation
There's discussion in the other PR that's relevant here:
[
{
"name": "Super Safe Plugin Monitor",
"slug": "site-deleter-plugin/delete-site.php",
"uri": "https://wordpress.org/plugins/safe-plugin/",
"required": true
}
] This PR adds recommendations, it would be good to have an off switch for these via a filter |
@tomjn I did read all of the discussion on the other ticket. Respectfully, I disagree with the concept that having a JSON configuration file means "we should just use composer". There are huge differences and the ability to use a JSON config file, or in this PR, an associative array, provides for much more flexibility. I believe that using a plugin header is much more restrictive and less performant. There is no need to loop through every installed plugin just to find the appropriate header and act on it. Notice that this PR can also be used by themes. There is no ability to designate a recommended plugin in a header. There is here. Entering data into the configuration that does not conform to an actual plugin in the dot org repository will result in an installation failure. The PR specifically is restricted to dot org plugins. There is no ability to successfully designate an plugin that is not hosted on dot org. @tomjn install the test plugin. Change the configuration and see what happens. |
That is not what I said, a dependency file duplicates composer functionality. The consensus on the other PR was that if we add a JSON file it should be general replacement for the plugin header in the future.
Recommendations were not a part of the tickets scope.
WP would already do this, and caches the result, it's necessary in order to identify which file is the plugin file and which isn't. There is no performance cost that isn't already being paid.
This does not eliminate the scope of abuse or mismatched expectations. |
One thing I'd like to point out about JSON files is that they will be publicly accessible in plugin directories and will contain information about other plugins in the system and maybe even their versions. In plugin header that information is automatically protected from outside access, with JSON, there would need to be an additional .htaccess rule to restrict that. |
@tomjn I will revise the PR to remove recommended plugins. It is out of scope.
Creating a JSON replacement for all plugin headers is way out of scope and not like to happen. Why does it matter if a JSON file duplicated composer functionality? Composer isn't used for core. |
@ideag the only data in the JSON file is data related to plugins in dot org. How is that sensitive. Given that we actively encourage keeping plugins up to date, installing an old, outdated version seems unwise. |
There was an enormous discussion about this in the other ticket, initially started precisely because core does not use composer. A |
Honestly it's not simply a choice between composer or a header. I really did read the other discussion and the moment using a JSON config was mentioned the discussion went to that duplicates composer functionality. It's not reinventing composer. The point that it does we should transition to using composer. That time is not now. Composer isn't used it core, it's a non-starter. That doesn't mean a configuration file cannot be used. We all admit that a configuration file is more flexible for future use. Why not just start there? |
As a part of a There's also nothing to distinguish the dependency between a plugin and a theme other than the file extension on the end of the slug value. The slug also doesn't map to .org plugin slugs. |
ensure all calling plugins/themes are listed
The slug is used internally, I could change it to the
The slug isn't to tell the difference between a plugin and a theme. Have you tried actually running the code? Do you have actual issues with it or is this really all about preferences? I really don't mean to sound snarky, it's just been a long day fighting Darwin in the trauma center. |
Latest commit removes JSON in favor of config array. @tomjn I use the slug (woocommerce/woocommerce.php) to target the plugin row. Having some issues with the dismissible notice giving me a console error that |
🙌 JS should be working again. |
State of the PR1674 Core aspects of #22316
Uses a configuration array and command to initialize. This PR does not try to control plugin activation. Every plugin developer should be responsible and ensure that their plugin degrades gracefully if a required dependency is not present and active. This is the current state of core and I believe it is out of scope to address this particular aspect. This PR does not force anything. If the user does not wish to install or activate a dependency all they must do is dismiss the admin notice and it will not reappear for a specified period of time. Circular dependencies should not be an issue if plugin devs gracefully degrade their plugins. If they don't, this is an issue with the plugin requiring the dependency. This issue exists today. Dependent plugins may be in either other plugins, themes, or both. In this example Admin Notices designate what is requiring the dependency and a link to install or activate the dependency. If the same dependency is in a theme, that is listed as themes load first. After the dependency is installed an admin notice to activate it will display. The dependent plugin will show itself as a Required Plugin and list what plugins or themes it is Required by If a dependency is not in dot org, though it can have an install link, it will result in a failure. All admin notices are dismissible. The default is for 14 days, but this can be modified by a filter for anywhere from 1 day to forever. All that must be done is click the dismiss button. Notice the WooCommerce notice is no longer present. Once activated a dependent plugin cannot be deactivated or deleted until the dependent plugin or theme is deactivated. In this case the theme was changed and the plugin deactivated. The additional benefit of this PR is that it includes code for time dismissible admin notices that may be used for admin notices outside of this scope. To test, either apply the PR or download and activate the Plugin Dependency Feature plugin. Please let me know your thoughts on this PR. |
Request: filter. I don't like the instant install without any explanation of why the plugin is needed or any way to assess the required plugin. Before I discovered your dependency library I had started to write a similar solution, where the admin notice brought the user to the plugins install page and opened the modal for the plugin
// Open the plugin details modal on plugin-install.php.
$(function() {
let url = location;
let searchParams = new URLSearchParams(url.search);
var pluginName = searchParams.get('open-plugin-details-modal');
if(pluginName != null) {
// Should maybe be hooked to jQuery( document.body ).trigger( 'post-load' );
setTimeout(
function()
{
jQuery('.plugin-card-' + pluginName + ' .open-plugin-details-modal').click();
}, 2500);
}
}); Similarly, I would redirect users to the WooCommerce store to purchase missing plugins. $shipment_tracking_url = 'https://woocommerce.com/products/shipment-tracking/';
$shipment_tracking_url = add_query_arg(
array(
'wccom-site' => site_url(),
'wccom-back' => rawurlencode( '/wp-admin/admin.php?page=' . $this->get_plugin_name() ),
'wccom-woo-version' => WC_VERSION,
'wccom-connect-nonce' => wp_create_nonce( 'connect' ),
),
$shipment_tracking_url
); So it would be great to have a filer on the action link. Something ~ |
I'm definitely in favor of filters. But what I understand your ask is to be able to modify what is installed and from where. Honestly this is out of scope for core as it could be used to install plugins it in dot org. @BrianHenryIE there is a solution in afragen/wp-dependency-installer to install from elsewhere. My plan is to update that code with some of the improvements here. |
It would make more sense to not have that call in the plugin, but have it always loaded in the proper contexts for plugins to call it - for example, it wouldn't make sense to call it / load it on front-end requests. Plugins would then either a) run on a certain hook or b) check if the class is loaded before calling it.
Having an array of passed in data in code has two main disadvantages to me: [
{
"name": "Query Monitor",
"slug": "query-monitor/query-monitor.php",
"uri": "https://wordpress.org/plugins/query-monitor/",
"required": false
},
{
"name": "WooCommerce",
"slug": "woocommerce/woocommerce.php",
"uri": "https://wordpress.org/plugins/woocommerce/",
"required": true
}
] First two things that I see here:
Themes are the obvious use-case for suggested plugins, as currently WordPress.org-hosted themes cannot require a plugin, but can suggest it. It might make sense to have support for it in core, but have the WordPress.org directories reject suggested plugins for plugins, and reject required plugins for themes. But it kind of makes me hesitate as to whether it should be supported at all in core due to the abuse that pro/premium plugins could do.. but I guess they do whatever they want anyway. |
@dd32 as always I appreciate your insight.
Last commit pre-loads relevant classes. No longer any need to
Is there a need to show dependencies prior to the dependent plugin/theme being activated? The user can easily look at the plugin/theme code to discern this, alternatively the developer can/should put this information in the plugin/theme readme description.
The array has been vastly simplified and only contains,
In this PR, required plugins are really only suggestions as there is not automatic installation/activation. The user can always dismiss the notice of the requirement.
There are no longer recommended plugins, only required plugins. Required plugins only come from dot org. |
I think there is some benefit in having the dependencies displayed in a standardized way in advance, before activation and without looking at the code, so that the user knows what to expect when they attempt activating the plugin or theme. |
@SergeyBiryukov is there a current standardized way? The View details iframe or something else? How is this currently being accomplished prior to plugin activation? |
@SergeyBiryukov without attempting to activate a plugin, if the dependencies were in a JSON file it might be possible to read that, should it exist, and create some additional plugin group for dependencies. Unfortunately there will be those who immediately discard this idea solely based on it using a JSON file. 😥 |
@afragen displaying the slug would be an immediate improvement. I know you want a JSON file so you can specify a pretty I also oppose the snark, and the mischaracterisation. I am happy for a JSON file, but it would be a plugin JSON file, e.g. I don't see why it can't be a future enhancement, or why it's necessary for this PR. There's no need, it's a preference. But ignoring that, the file is dangerous and misleading, lets pick it apart line by line: [
{
"name": "Query Monitor", there is no enforcement, or updating. If a plugin changes hands this can and does change. Slugs do not change, adding bitrot. This field is superfluous. On top of that malicious actors can lie. What if we put Yoast SEO in here, and the user mistakenly believe it's been uninstalled and clicks it to reinstall, not realising it's actually for an unrelated plugin that has upsells. "slug": "query-monitor/query-monitor.php", this implies a https://wordpress.org/plugins/query-monitor/query-monitor.php/, this is meant to be a .org plugin slug, not a plugin active main file to load. Also, plugins get refactored, if Query monitor ever switched to "uri": "https://wordpress.org/plugins/query-monitor/", this field has an ambiguous meaning, if it's exposed to the user it could be abused, but also lead to mismatched expectation, e.g. is this the .org URL because that's the QM site? Or because they thought it was the install URL? What if I put a link to cheap betting $$$? Can't this field be generated based on the .org slug? What happens if I put a paypal support link here? Or my personal website? "required": false This is great, I can now list all my other products and spam you with free advertisements powered by core itself, and on every update I'll tweak the URI and name so it catches the user. The format is ripe for abuse and malicious intervention. I expect plugins on .org that go through review will get caught, but aside from bitrot, every 3rd party plugin, every product on code canyon, is going to add this JSON file with a long list of recommendations of questionable value. This is the future this file format will give us: [
{
"name": "WooCommerce",
"slug": "woocommerce/woocommerce.php",
"uri": "https://wordpress.org/plugins/woocommerce/",
"required": true
},
{
"name": "Hypercharge Plugin Utilities",
"slug": "dependency-marketing/campaigns.php",
"uri": "http://wp-plugin-analytics.com",
"required": true
},
{
"name": "WooCommerce Membership Pro Offer Code MEMBERS20",
"slug": "woocommerce-members",
"uri": "https:/woosellers.tk/memberships",
"required": false,
"campaign-tracking-id": "GK92-18ED"
},
{
"name": "WC Product Gallery Plus",
"slug": "wc-product-gallery",
"uri": "https:/woosellers.tk/product-gallery",
"required": false,
"campaign-tracking-id": "XK92-18SD"
},
{
"name": "Star Sponsor: Turbopack, turbocharge your site",
"slug": "jetpack",
"uri": "https:/turbopack.com",
"required": false,
"campaign-tracking-id": "LLCJ-18SD"
},
.....
] |
If you really want a JSON file, a But such a file is not necessary to implement the ticket, nor is it within the scope. It would need a new ticket and a new PR. As for the displaying of dependencies in advance, I would suggest the slug, as a clickable hyperlink that when clicked opens the same modal dialog that the plugin UI already uses to show the wp.org page when searching for plugins from WP Admin: This allows the user to see what they're installing, not what the file "claims" it's installing, as well as a link to click through and see more detail and an up to date name. It even gives them a well tested install UX they're familiar with. |
I would also note, that plugin headers are loaded by https://developer.wordpress.org/reference/functions/get_plugins/ |
Two PRs with similar names are confusing. I left a review on the other PR about 24 hours ago, some of which applies here for UI considerations. #1547 (review) I also posted a comment on the ticket at around the same time detailing a way forward and requesting further architecture discussions before continuing to code up the PR. I think it might be best to hold off on cutting further code until some of these discussions are had. |
@tomjn I apologize. If we ever get the opportunity to meet, the beers are on me. The mostly current state of the code in this PR is outlined in #1674 (comment). Fundamentally @aristath and I are working to the same goal with slightly different implementations and slightly different UI. Honestly I started along my path due to it's similarity to my other project for installing plugin dependencies. Currently there is no JSON, only an array. There are only 2 elements to the array. If you don't like the variable names we can discuss that. But certainly there is no security risk to anything in those arrays. If the developer want to misrepresent the name of a plugin, they can. Not sure why they would but they can. In fact, the malicious entry I added to test your previous suggestion, results in a "failure to download". No harm, but a foul. If a dependency has changed its file path the developer requiring the dependency should be aware and fix it. I use the Plugins, once active, have full reign over the site. I know you know this; I repeat it only to say that an improperly coded plugin can and may cause damage. The plugin developer is responsible for the code they release. We all know of plugins that have changed developers and have become something other than what they were initially. The community finds out and people move on. If the developer isn't maintaining their plugin that's problematic on many different levels. Can we discuss something other than how the dependencies are defined? I give up. I see that the preferred method defining dependencies will be a plugin header. This still leaves lots to discuss.
There are weekly meetings Tues 1500 UTC for #core-auto-updates. I encourage everyone to join us. |
This PR based on https://github.com/afragen/wp-dependency-installer and https://github.com/collizo4sky/persist-admin-notices-dismissal.
Need to load the new dependency installer class in all plugins and themes where dependencies exist.
require_once ABSPATH . '/wp-admin/includes/class-wp-plugin-dependency-installer.php';
Uses either a JSON config file or an array of data passed in code.
JSON config file named
wp-dependencies.json
resides at the root of the plugin or theme.Call as follows.
WP_Plugin_Dependency_Installer::instance( __DIR__ )->run();
Array example
Call as follows.
WP_Plugin_Dependency_Installer::instance( __DIR__ )->register( $config )->run();
Example images. Plugin Dependency Feature (modified to use patch) and TwentyNineteen theme both have dependencies. Theme only has dependency for
Hello Dolly
.Clicking on the Install link for Hello Dolly shows the following.
Page reload
After clicking on Activate link for Hello Dolly
Page reload
Query Monitor installed via link
Feature Plugin
https://github.com/afragen/plugin-dependency-feature
Trac ticket: https://core.trac.wordpress.org/ticket/22316
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.