Make WordPress Core

Opened 11 years ago

Last modified 4 years ago

#22287 new defect (bug)

Plugin in another plugin folder causes Activate link to be wrong on Download

Reported by: joehoyle's profile joehoyle Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.4
Component: Upgrade/Install Keywords: dev-feedback has-patch
Focuses: Cc:

Description

I am not sure if shipping a plugin within another plugin is officially supported, but there is an inconsistency between the activate links in the Manage Plugins page, and the Activate Plugin on the Plugin Downloaded success page.

This is because on the Manage Plugins page, the get_plugins() is going to scan the plugins dir and all dirs within it, however, on the Upgrade page, it calls get_plugins() specifying the plugin dir as the base (see https://github.com/WordPress/WordPress/blob/master/wp-admin/includes/class-wp-upgrader.php#L579)

That will cause the "embedded" plugin to be picked up, and if it's alphabetically above the main plugin file (presumably) see code comment " Assume the requested plugin is the first in the list"

Attachments (2)

class-wp-upgrader.php.diff (1.0 KB) - added by joehoyle 11 years ago.
22287.diff (962 bytes) - added by pbiron 4 years ago.

Download all attachments as: .zip

Change History (26)

#1 @joehoyle
11 years ago

Perhaps this could be fixed by adding a second param "$search_directories" to get_plugins() which is set to false in the Upgrader step?

I am happy to write a patch with some direction on this.

#2 @joehoyle
11 years ago

I added a patch that will order the array from get_plugins( $plugin ) with files not in directories to the top.

#3 @dd32
11 years ago

  • Component changed from Plugins to Upgrade/Install

Duplicate of #21954, not closing as the other ticket doesn't have a patch.
I like the patch, and it's the preferred solution I suggested in that ticket too.

#4 @joehoyle
11 years ago

Yes, seems we had the same idea on both accounts, who do I need to bug to try get this committed?

#5 @SergeyBiryukov
11 years ago

#21954 was marked as a duplicate.

#6 @SergeyBiryukov
11 years ago

From #21954:

This issue also applies when deleting the plugin as it lists both plugins on the confirmation page.

#7 @SergeyBiryukov
11 years ago

  • Version changed from trunk to 3.4

#8 @jdgrimes
10 years ago

What is the benefit of this solution over not looking the the subdirectories unnecessarily? Adding a second parameter as suggested above would be backward compatible (if set to true be default), and wouldn't require the site to do unnecessary work.

#9 @jdgrimes
10 years ago

#26368 was marked as a duplicate.

#10 @toscho
10 years ago

  • Cc info@… added

#11 @johnbillion
10 years ago

  • Cc johnbillion added

#12 @jdgrimes
10 years ago

Interestingly, in Plugin_Upgrader::check_package(), the directory is just globed for .php files: https://core.trac.wordpress.org/browser/tags/3.9.1/src/wp-admin/includes/class-wp-upgrader.php#L686

So it seems there is an inconsistency with whether get_plugins() is even used.

#13 @jdgrimes
10 years ago

Somewhat unrelated sidenote: glob() may actually be faster than the current opendir()/readdir() method used in get_plugins(), in addition to being much less code (and therefore more readable).

#14 @chriscct7
8 years ago

  • Keywords needs-patch added

#15 @dd32
8 years ago

#35277 was marked as a duplicate.

This ticket was mentioned in Slack in #core by arunas. View the logs.


7 years ago

#17 @ideag
7 years ago

Hi! I just ran into this issue and I'd like to help closing it, but I am wondering what would be the preferred solution for it. I would probably prefer modifying get_plugins to optionally ignore sub-directories, but I've also seen suggestions to modify the order of output to make sure files directly in plugin directory are at the top of the list.

#18 follow-up: @jdgrimes
7 years ago

  • Keywords dev-feedback added

@ideag Yeah, it seems that there are really three solutions:

  • Sort the results from get_plugins().
  • Add a parameter to get_plugins() to skip checking in subdirectories.
  • Don't use get_plugins() at all, as in Plugin_Upgrader::check_package().

I favor the second option, since it avoids looking into the subdirectories unnecessarily. (The third option would do that as well, however, it would require duplicate logic.)

But maybe @dd32 and @SergeyBiryukov would like to give their opinion.

#19 in reply to: ↑ 18 @ideag
7 years ago

Replying to jdgrimes:

I favor the second option, since it avoids looking into the subdirectories unnecessarily. (The third option would do that as well, however, it would require duplicate logic.)

I also think the second option would be the best way to go and has the least chance of having unintended side-effects. I'll try to put together a patch and some unit tests over the weekend.

#20 @ocean90
4 years ago

#48398 was marked as a duplicate.

#21 @ocean90
4 years ago

Both, Plugin_Installer_Skin and Plugin_Upgrader_Skin, are using Plugin_Upgrader::plugin_info().

#22 @pbiron
4 years ago

  • Keywords has-patch needs-refresh added; needs-patch removed

This ticket was mentioned in Slack in #core-auto-updates by pbiron. View the logs.


4 years ago

@pbiron
4 years ago

#24 @pbiron
4 years ago

  • Keywords needs-refresh removed

22287.diff refreshes class-wp-upgrader.php.diff so that applies cleanly after all these years :-)

While patch solves the case of the plugin in #21954 (i.e., when the "additional" plugin is in a sub-directory), it's also possible to have multiple files with plugin headers directly in the plugin's dir...and the "main" plugin in that case may not necessarily be the one that is alphabetically first.

One example of a plugin that has 2 files with plugin headers at the "root" of the plugin's directory is https://meta.trac.wordpress.org/browser/sites/trunk/wordpress.org/public_html/wp-content/plugins/handbook

So, maybe a heuristic for that case would be to first check for a file named {$slug}.php, where $slug is the plugin's directory name. If that file exists, use it; if not, then do the sort and return as in the patch.

Thoughts?

Note: See TracTickets for help on using tickets.