Make WordPress Core

Opened 14 years ago

Closed 13 years ago

#13550 closed enhancement (wontfix)

WP_PLUGIN_DIR doesn't detect symlinks

Reported by: acumensystems's profile acumensystems Owned by: westi's profile westi
Milestone: Priority: normal
Severity: minor Version: 3.0
Component: Plugins Keywords: has-patch 3.2-early
Focuses: Cc:

Description

Hi

I've noticed a number of plugins hardcoding their plugin directories, and I think I see why. If you develop a theme, and that theme depends on plugins, it's tempting to bundle the plugins along with the theme files (i.e. all in wp-content/themes/mytheme).

Then, to allow Wordpress to use these plugins, it's common to symlink wp-content/plugins to wp-content/themes/mytheme/plugins, or somesuch.

That means that plugins using plugin_basename(FILE) will break, as the WP_PLUGIN_DIR path will not be removed from the full returned path.

I've written a patch for wp-includes/default-constants.php that will detect if the plugins directory is a symlink, and if so will use the appropriate correct path as part of the constant, thereby ensuring all paths calculated by plugins are correct based on this constant.

Attachments (2)

Basename.patch (748 bytes) - added by acumensystems 14 years ago.
Patch for wp-include/default-constants.php
plugin_basename.patch (556 bytes) - added by vladimir_kolesnikov 13 years ago.
Adds a call to a filter to override plugin_basename()'s result

Download all attachments as: .zip

Change History (24)

@acumensystems
14 years ago

Patch for wp-include/default-constants.php

#1 follow-up: @dd32
14 years ago

  • Keywords symlink plugin basename paths removed
  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Using simlinks is not a good idea here, as you've found out.

Instead, You should be adding a define to your wp-config.php to direct WordPress to the true folder:

define('WP_PLUGIN_DIR', ABSPATH . '/wp-content/themes/my-theme/plugins');
define('WP_PLUGIN_URL', 'http://.....');

I'm closing this as wontfix due to the availability of the above constants to define locations such as these. I think you'll find the reason plugins hard-code paths is due to either lack of developer knowledge, or simply old un-updated plugins which were around before the new API was introduced for such functionalities.

(As an aside, That patch will probably not work.. ending up as /home/..../wp-content//home/..../wp-content/themes..../ readlink() returns an absolute path i believe.)

#2 @acumensystems
14 years ago

  • Cc acumensystems added

Hi

Yes, readlink does return an absolute path, which is what the WP plugin directory will often and happily be. The problems I was describing use plugin_basename, and an absolute path is very much what they expect. I have tested the patch working, you can recreate by adding the symlink I described.

The variable returned by readlink, and the many of the checks in plugin_basename are simply to help "remove" parts of the path that are not wanted in the final plugin path to be returned. Without my patch, plugin_basename will return a double length path containing everything before the symlink twice, and everything after it once. Simply, the result is incorrect, and if WP can not determine the path, it should return false and be error-handled.

I do appreciate that these constants are available, but I do also think that the assumption made by WP could be a little more robust, as it is a common approach for developers to use symlinks to get all their "dynamic" resources in one place, if the structure of software needs them from various subdirectories at different levels of the main project tree - i.e. /wp-content/uploads, wp-content/themes, and plugins/ are the dynamic paths here.

So I feel this patch helps work around what would otherwise be the suggestion that all "user" data for WP is kept in a single common parent directory. I appeal for you to test and validate my patch as it only adds to the robustness of WPs behaviour, and it does not decrease efficiency or complexity in any significant way.

Thanks for your time,
Leo

#3 in reply to: ↑ 1 @shallot
14 years ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

Replying to dd32:

Using simlinks is not a good idea here, as you've found out.

Instead, You should be adding a define to your wp-config.php to direct WordPress to the true folder:

define('WP_PLUGIN_DIR', ABSPATH . '/wp-content/themes/my-theme/plugins');
define('WP_PLUGIN_URL', 'http://.....');

I have a system-wide Wordpress installation, with some system-wide plugins, but I need to allow individual (virtual host) users to have some of their own plugins. At the same time they remain their own responsibility, so I don't want to just copy them over to the system-wide directory, I just symlink their particular plugins to their particular directories.

This basically works. However, wp-includes/plugins.php's plugin_basename() has no facility to allow for it, it only removes the WP_PLUGIN_DIR or WPMU_PLUGIN_DIR from the paths, not the custom destination paths. In wp-admin/includes/plugin.php the _registered_pages global array gets filled with inconsistent values as get_plugin_page_hookname() first gets called with the readlink'd plugin file path and later with the original one relative to the systemwide directory, which in turn makes user_can_access_admin_page() drop all plugin settings page requests.

If I were to reverse the directory layout and use a custom WP_PLUGIN_DIR for each virtual host, then I would again have to copy over all the common plugins into each of them - the same logic would prevent me from symlinking them.

Wouldn't it be useful to simply add an optional array of custom plugin directories that plugin_basename() could add to its list for pruning?

#4 @shallot
14 years ago

To put this in terms of code, this is what I added after the preg_replace() in wp-includes/plugin.php's plugin_basename()

<pre>
if (defined('WP_EXTRA_PLUGIN_DIR'))

$file = preg_replace('#' . preg_quote(WP_EXTRA_PLUGIN_DIR, '#') . '/#',,$file);

</pre>

Now our settings pages work fine.

#5 @holizz
14 years ago

  • Cc tom@… added

#6 @vladimir_kolesnikov
13 years ago

this is what I added after the preg_replace() in wp-includes/plugin.php's plugin_basename()

Maybe it's worth adding something like

    return apply_filters('plugin_basename', $file);

to the end of plugin_basename() function? This will be a more general solution than to define a constant.

@vladimir_kolesnikov
13 years ago

Adds a call to a filter to override plugin_basename()'s result

#7 @scribu
13 years ago

  • Keywords has added
  • Milestone set to 3.1

If I'm not mistaken, Basename.patch only treats the case where the entire plugin dir is symlinked and not when an individual plugin is.

So, yeah, I would go with plugin_basename.patch

#8 @scribu
13 years ago

  • Keywords has-patch added; has removed

#9 follow-up: @scribu
13 years ago

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

(In [15829]) Apply filter in plugin_basename(). Props vladimir_kolesnikov. Fixes #13550

#10 in reply to: ↑ 9 ; follow-up: @westi
13 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to scribu:

(In [15829]) Apply filter in plugin_basename(). Props vladimir_kolesnikov. Fixes #13550

This doesn't really solve this issue and just introduces more issues.

Now we have one of the plugin_ functions working in this case and the others don't.

Also how are these plugins going to interact with the built-in upgrader.

I think it would be better not to introduce this filter as it isn't really a solution and there are better ways to support sharing plugins across multiple WP installs.

#11 follow-up: @scribu
13 years ago

Now we have one of the plugin_ functions working in this case and the others don't.

Please provide an example.

Also how are these plugins going to interact with the built-in upgrader.

I think it would be better not to introduce this filter as it isn't really a solution and there are better ways to support sharing plugins across multiple WP installs.

I assume you're referring to #15134

This filter isn't useful only for that. For example, with it, I'm able to have svn/my-plugin/trunk symlinked to plugins/my-plugin and have activation hooks work.

#12 in reply to: ↑ 10 @vladimir_kolesnikov
13 years ago

Now we have one of the plugin_ functions working in this case and the others don't.

Well, smth like that (oversimplified):

function my_plugin_basename($file)
{
    static $my_paths = array('var/www/storage1/', 'var/www/storage2');
    foreach ($my_paths as $path) {
        $file = str_replace($path, '', $file);
    }

    return $file;
}

add_filter('plugin_basename', 'my_plugin_basename');

$my_paths lists all possible destinations for the shared plugin locations and in the loop we remove them from $file (the real logic will be more complicated). The result will be plugin_dir/plugin_file — basically what plugin_basename() returns for the plugins located in WP(MU)_PLUGIN_DIR.

register_XXX_hook(), add_XXX_page(), plugins_dir/url() functions all rely upon plugin_basename() and with my_plugin_basename hook installed the resulting path/url will be correct.

Also how are these plugins going to interact with the built-in upgrader.

What do you mean?

Wrt to #15134, the upgrader will remove the symlink only from the blog being updated leaving all others unaffected.

there are better ways to support sharing plugins across multiple WP installs.

I'd really love to know about them. But in the case when only some plugins are shared between the blogs I cannot imagine other solution than symlinking.

#13 in reply to: ↑ 11 ; follow-up: @westi
13 years ago

Replying to scribu:

Now we have one of the plugin_ functions working in this case and the others don't.

Please provide an example.

plugins_url builds a url to a file based on the known locations of the mu-plugins directory and the WPMU_PLUGIN_URL and WP_PLUGIN_URL constants.

This won't work for plugins located elsewhere.

#14 in reply to: ↑ 13 @vladimir_kolesnikov
13 years ago

Replying to westi:

This won't work for plugins located elsewhere.

Symbolic link is not an issue here. Technically the plugin is still accessible under /wp-content/plugins/<plugin_name>

The problem with plugin_basename() was that when you are passing FILE as its argument, PHP passes the real path of the file, not the symlinked one.

That is, if you have this:

/wp-content/plugins/test.php is a symlink to /some/other/path/test.php

FILE in test.php will always be /some/other/path/test.php which breaks plugin_basename() because it expected to see /wp-content/plugins/test.php. However, since test.php is accessible from both locations, plugins_url() function still returns the correct result.

#15 @willnorris
13 years ago

would it not make sense to store the original passed-in value of $file and pass that as an optional second argument to the plugin_basename filter? Otherwise, you're only ever able to see the "mangled" version. Admittedly, I don't have an immediate use-case in mind, but that often seems to be best practice with filters of this type.

#16 @markjaquith
13 years ago

(In [16575]) Revert [15829] for now, consider better solution for 3.2. see #13550

#17 @scribu
13 years ago

I don't see what's the problem with the current solution. plugins_url(), as well as all the other related functions pass through plugin_basename(), so it's consistent.

I can pass the original path as willnorris suggests, if that was the issue.

#18 @scribu
13 years ago

  • Keywords 3.2-early added
  • Milestone changed from 3.1 to Future Release

#19 @hd-J
13 years ago

  • Cc contact@… added

#20 @coffee2code
13 years ago

  • Cc wptrac@… added

#21 @jltallon
13 years ago

  • Cc jltallon added

Some more input on this:

We normally use the debian-provided package version, which is installed (system-wide) at /usr/share/wordpress.

For each WP installation, we define('WP_CONTENT_DIR','/srv/customer-domain/wordpress') or the like, and WP_CONTENT_URL accordingly.
In order to keep a sane environment, we usually symlink plugins (within WP_CONTENT_DIR/plugins) to their sytem-wide location.

For plugin-provided downloadable files (such as JS or CSS), the best solution we have come up with so far is to do:

$url = plugins_url( basename(dirname(__ FILE __)).'/path/to/file');

which could be expressed as:

$url = WP_PLUGIN_URL.plugin_basename(dirname(__ FILE __).'/path/to/file'

This can an issue when __FILE__'s path is not precisely WP_CONTENT_DIR .'/'. <plugin_basename> .'/'. <filename>, since plugin_basename is actually a symlink.

I'm available to provide more input on this.

Last edited 13 years ago by scribu (previous) (diff)

#22 @scribu
13 years ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

Closing this ticket as wontfix, since it focuses on the wrong thing.

See #16953.

Note: See TracTickets for help on using tickets.