Make WordPress Core

Opened 10 years ago

Closed 7 years ago

#28441 closed defect (bug) (fixed)

Errant activation hook with symlinked plugins in WordPress 3.9.x

Reported by: clifgriffin's profile clifgriffin Owned by: rmccue's profile rmccue
Milestone: 4.6 Priority: normal
Severity: normal Version: 3.9
Component: Plugins Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

It seems there is still a scenario where symlinked plugins do not work seamlessly in WordPress 3.9.1.

Here is my setup:

Plugin location on disk:
/Users/me/Dropbox/Development/Repositories/shopp-arrange/trunk

Symlinked location:
/Applications/MAMP/htdocs_test/wp-content/plugins/trunk

When running ls in the plugins directory, it returns this in terminal:
trunk -> /Users/me/Dropbox/Development/Repositories/shopp-arrange/trunk/

Additionally, $wp_plugin_paths contains the following entry:

Applications/MAMP/htdocs_test/wp-content/plugins/trunk => /Users/me/Dropbox/Development/Repositories/shopp-arrange/trunk

Which looks exactly right. wp_register_plugin_realpath seems to be doing its job.

The issue is that my activation hook isn't running. I finally decided to look at the actions queue and see what's registered there. I found:

[activate_shopp-arrange/trunk/shopp-arrange.php] => Array
        (
            [10] => Array
                (
                    [000000002f54486c000000013e83cf29activate] => Array
                        (
                            [function] => Array
                                (
                                    [0] => ShoppArrange Object
                                        (
                                            [table_name] => wp_cgd_term_order
                                            [prefix] => shopp_arrange
                                            [settings] => 
                                        )

                                    [1] => activate
                                )

                            [accepted_args] => 1
                        )

                )

        )

This does not look correct at all.

After spending a couple of hours looking at this, I decided to temporarily give up the search and report the issue.

Attachments (3)

wp-plugin-paths.php (2.8 KB) - added by jdgrimes 9 years ago.
Example solution, implmented as a static class
28441.patch (1.5 KB) - added by jdgrimes 9 years ago.
Sort the paths with arsort() before resolving symlinks
28441.2.patch (2.3 KB) - added by ocean90 8 years ago.

Download all attachments as: .zip

Change History (19)

#1 follow-up: @jdgrimes
10 years ago

I can only reproduce this by adding this:

$wp_plugin_paths['/Applications/MAMP/htdocs_test/wp-content/plugins'] = '/Users/me/Dropbox/Development/Repositories';

I have a feeling you probably have a plugin with this path: /Users/me/Dropbox/Development/Repositories/my-plugin.php which you are also symlinking to the /Applications/MAMP/htdocs_test/wp-content/plugins directory.

The solution is that the $wp_plugin_paths should always be ordered by length, from longest to shortest. That way we won't have path conflicts like this.

Related: #16953

#2 in reply to: ↑ 1 @clifgriffin
10 years ago

Replying to jdgrimes:

I can only reproduce this by adding this:

$wp_plugin_paths['/Applications/MAMP/htdocs_test/wp-content/plugins'] = '/Users/me/Dropbox/Development/Repositories';

I have a feeling you probably have a plugin with this path: /Users/me/Dropbox/Development/Repositories/my-plugin.php which you are also symlinking to the /Applications/MAMP/htdocs_test/wp-content/plugins directory.

The solution is that the $wp_plugin_paths should always be ordered by length, from longest to shortest. That way we won't have path conflicts like this.

Related: #16953

Unfortunately, no. Just double checked to be certain, but I only have symlinks setup for directories. From terminal:

mbp:plugins me$ ls -lart | grep "\->"
lrwxr-xr-x   1 me  admin      79 Feb  8 11:12 advanced-content-templates -> /Users/me/Dropbox/Development/Repositories/advanced-content-templates/
lrwxr-xr-x   1 me  admin      65 Feb  8 11:48 wp-sent-mail -> /Users/me/Dropbox/Development/Repositories/wp-sent-mail/
lrwxr-xr-x   1 me  admin      66 Feb  8 11:48 genesis-shopp -> /Users/me/Dropbox/Development/Repositories/genesis-shopp/
lrwxr-xr-x   1 me  admin      79 Feb  8 12:55 simple-post-template -> /Users/me/Dropbox/Development/Repositories/simple-post-template/trunk/
lrwxr-xr-x   1 me  admin      75 Feb 11 23:28 shopp-google-analytics -> /Users/me/Dropbox/Development/Repositories/shopp-google-analytics/
lrwxr-xr-x   1 me  admin      72 Apr  3 20:32 shopp-exchange-rates -> /Users/me/Dropbox/Development/Repositories/shopp-exchange-rates
lrwxr-xr-x   1 me  admin      64 May  6 07:56 featherlinks -> /Users/me/Dropbox/Development/Repositories/featherlinks
lrwxr-xr-x   1 me  admin      57 May 21 13:43 shopp -> /Users/me/Dropbox/Development/Repositories/shopp
lrwxr-xr-x   1 me  admin      66 May 27 19:37 shopp-reviews -> /Users/me/Dropbox/Development/Repositories/shopp-reviews/

#3 follow-up: @jdgrimes
10 years ago

This one does the same thing, in effect:

shopp -> /Users/me/Dropbox/Development/Repositories/shopp

FWIW, my test code adapted from plugin_basename():

$file = '/Users/me/Dropbox/Development/Repositories/shopp-arrange/trunk/shopp-arrange.php';

$wp_plugin_paths['/Applications/MAMP/htdocs_test/wp-content/plugins/shopp'] = '/Users/me/Dropbox/Development/Repositories/shopp';
$wp_plugin_paths['/Applications/MAMP/htdocs_test/wp-content/plugins/trunk'] = '/Users/me/Dropbox/Development/Repositories/shopp-arrange/trunk';

foreach ( $wp_plugin_paths as $dir => $realdir ) {
	if ( strpos( $file, $realdir ) === 0 ) {
		$file = $dir . substr( $file, strlen( $realdir ) );
	}
}

$plugin_dir = '/Applications/MAMP/htdocs_test/wp-content/plugins';
$mu_plugin_dir = 'doesntmatterwhat';
$file = wp_normalize_path( $file );
$plugin_dir = wp_normalize_path( $plugin_dir );

$file = preg_replace('#^' . preg_quote($plugin_dir, '#') . '/|^' . preg_quote($mu_plugin_dir, '#') . '/#','',$file); // get relative path from plugins dir
$file = trim($file, '/');
echo $file;
exit;

Result: shopp-arrange/trunk/shopp-arrange.php

#4 in reply to: ↑ 3 ; follow-up: @clifgriffin
10 years ago

Oh man, I didn't notice the trailing slash on most of those. I remember vaguely there is a difference when symlinking, but it didn't occur to me since everything else was working properly.

So basically, instead of symlinking with this:

ln -s ~/Dropbox/Development/Repositories/shopp-arrange/trunk/ ./shopp-arrange

I should have done:

ln -s ~/Dropbox/Development/Repositories/shopp-arrange/trunk/ ./shopp-arrange/

?

#5 in reply to: ↑ 4 @jdgrimes
10 years ago

  • Version changed from 3.9.1 to 3.9

No, it doesn't make any difference. The paths will always be without the trailing slash after WordPress formats them.

#6 follow-ups: @rmccue
10 years ago

  • Owner set to rmccue
  • Status changed from new to assigned

I'll take a look into this. :)

#7 in reply to: ↑ 6 @clifgriffin
10 years ago

Replying to rmccue:

I'll take a look into this. :)

Thanks :)

If you need any other information, let me know. I'm going to do some experimentation with xdebug today, if I can find time.

#8 @DrewAPicture
9 years ago

  • Component changed from General to Plugins

@jdgrimes
9 years ago

Example solution, implmented as a static class

#9 @jdgrimes
9 years ago

FWIW, wp-plugin-paths.php is an example solution. It is implemented as new static class because it is based off of a solution from a project of mine that had a similar issue. The wp_register_plugin_realpath() function is replaced by WP_Plugin_Paths::register() and the code in plugin_basename() that currently resolves the paths is replaced by WP_Plugin_Paths::resolve().

The reason this was originally a class is to avoid a global variable. And it was also more efficient, because it allows us to use the ksort() function, and only to sort the paths when it is needed.

For backward compatibility with the current implementation, we may want to keep using a global variable instead. However, that will also mean that we'll need to sort the paths each time we need to resolve them, and use a different, much less efficient method of sorting the paths. That is, unless we change the array structure of the global, which would be backward incompatible for anyone hacking $wp_plugin_paths anyway.

So as a summary of this solution, if implemented as a class like this:

Pro: it will be much more efficient at resolving the paths than otherwise (I don't know how significant this is though, I haven't benchmarked anything).

Con: it isn't backward compatible for anyone hacking $wp_plugin_paths.

@jdgrimes
9 years ago

Sort the paths with arsort() before resolving symlinks

#10 @jdgrimes
9 years ago

  • Keywords has-patch added

The solution is actually much simpler than I originally thought. I've attached 28441.patch, which modifies plugin_basename() to sort the paths with arsort() before resolving any symlinks. This sorts the paths reverse-alphabetically while preserving the keys. This works because it results in a longer path being listed before a shorter one with the same base directory(ies). (If anyone knows of a better array sorting function to use here, I'm all ears.)

I've included a unit test which demonstrates the fix.

#11 in reply to: ↑ 6 @jorbin
9 years ago

Replying to rmccue:

I'll take a look into this. :)

Find anything?

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


8 years ago

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


8 years ago

#14 @ocean90
8 years ago

  • Keywords needs-refresh added
  • Milestone changed from Awaiting Review to 4.6

Patch needs a refresh after [37332].

@ocean90
8 years ago

#15 @ocean90
8 years ago

  • Keywords has-unit-tests added; needs-refresh removed

#16 @ocean90
7 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 37983:

Plugins: In plugin_basename() sort plugin paths before resolving symlinks.

arsort() sorts the paths reverse-alphabetically while preserving the keys. It results in a longer path being listed before a shorter one with the same base directory(ies).

Props jdgrimes, ocean90.
Fixes #28441.

Note: See TracTickets for help on using tickets.