Make WordPress Core

Opened 14 years ago

Closed 11 years ago

Last modified 11 years ago

#16953 closed enhancement (fixed)

Allow symlinked plugins

Reported by: scribu's profile scribu Owned by: rmccue's profile rmccue
Milestone: 3.9 Priority: normal
Severity: normal Version:
Component: Plugins Keywords: needs-patch
Focuses: Cc:

Description (last modified by scribu)

There are many scenarios where one would like to have a plugin's folder symlinked to another location.

A couple of these scenarios are described in #13550.

However, when using symlinks, code such as this fails:

plugins_url( 'script.js', __FILE__ );

This happens because __FILE__ resolves to the real path, which confuses plugin_basename().

The most simple and most flexible solution is to add a filter to plugin_basename(), leaving individual devs to handle symlinked paths, depending on their environment.

Attachments (22)

16953.diff (552 bytes) - added by scribu 14 years ago.
'pre_plugin_basename' filter
test.zip (2.1 KB) - added by scribu 14 years ago.
WP_fix_for_plugin_basename_to_allow_symlinks_2011-07-28.patch (711 bytes) - added by augustash 13 years ago.
Fix for correctly extracting the local directory and filename regardless if the file comes from a symlink or not
WP_fix_for_plugin_basename_to_allow_symlinks_2011-07-28.2.patch (1.8 KB) - added by augustash 13 years ago.
16953.alternative.diff (2.7 KB) - added by mitchoyoshitaka 12 years ago.
Alternative approach, which caches symlink targets and replaces them
symlinked_plugins.diff (2.6 KB) - added by MikeSchinkel 12 years ago.
Patch to enable plugin_url() to support symlinked plugins.
symlinked_plugins-2.diff (2.3 KB) - added by MikeSchinkel 12 years ago.
2nd attempt at patch to enable plugin_url() to support symlinked plugins.
pre-plugin-basename.diff (673 bytes) - added by MikeSchinkel 11 years ago.
pre-plugin-basename.diff
16953.2.diff (891 bytes) - added by nacin 11 years ago.
16953.3.diff (2.9 KB) - added by rmccue 11 years ago.
First pass at using realpath to find symlinks
16953.4.diff (3.0 KB) - added by rmccue 11 years ago.
Again, with a filter as well
16953.5.diff (3.2 KB) - added by rmccue 11 years ago.
Once more with more pizazzHHHH documentation
16953.4.2.diff (3.2 KB) - added by rmccue 11 years ago.
Once more with more pizazzHHHH documentation and correct variable names
16953.6.diff (3.2 KB) - added by rmccue 11 years ago.
Where you get all them documentation standards
16953.7.diff (4.0 KB) - added by rmccue 11 years ago.
Check on activation as well
16953.8.diff (8.2 KB) - added by MikeSchinkel 11 years ago.
Added wp_register_plugin_realpath() in uninstall_plugin() to 16953.7.diff
16953.9.diff (5.2 KB) - added by jdgrimes 11 years ago.
Add network plugin support in multisite
16953.10.diff (649 bytes) - added by iandunn 11 years ago.
Check if $wp_plugin_paths is an array
no-mu-symlinks.diff (350 bytes) - added by scribu 11 years ago.
mu-symlinks.diff (884 bytes) - added by jdgrimes 11 years ago.
Resolve MU-plugin and single file plugin symlinks
16953.no-single-file.diff (923 bytes) - added by rmccue 11 years ago.
Ignore mu-plugins and single-file plugins
16953.no-single-file.2.diff (1.3 KB) - added by rmccue 11 years ago.
Ensure we normalise base plugin directory paths

Download all attachments as: .zip

Change History (149)

@scribu
14 years ago

'pre_plugin_basename' filter

#1 @scribu
14 years ago

  • Keywords has-patch added
  • Owner set to scribu
  • Status changed from new to accepted

Test files coming up.

@scribu
14 years ago

#2 @scribu
14 years ago

  • Description modified (diff)
  • Keywords commit added
  • Milestone changed from Awaiting Review to 3.2

To test:

  1. Extract test.zip into a wp root directory
  2. Activate Test Plugin

The archive already contains a symlink, so it probably won't work on Windows out of the box.

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

#3 @scribu
14 years ago

  • Description modified (diff)

#4 @scribu
14 years ago

  • Description modified (diff)

#5 @scribu
13 years ago

  • Description modified (diff)

#6 @westi
13 years ago

  • Milestone changed from 3.2 to Future Release

Not a 3.2 feature moving to Future Release.

#7 @scribu
13 years ago

  • Milestone Future Release deleted
  • Resolution set to worksforme
  • Status changed from accepted to closed

Well what'ya know... there's already a 'plugins_url' filter which can be used just as well.

#8 @scribu
13 years ago

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

Found a case that's not covered by the 'plugins_url' filter: register_*_hook().

#9 @ciantic
13 years ago

Yes, very annoying, it boils down to lack of feature in PHP, see PHP bug 46260 (I actually believe best way to fix PHP bug is to create new constant like __FILELINK__ to keep backwards compatibility). But getting that fixed anytime soon is not reality.

@scribu, can you elaborate what kind of problems you encountered using plugins_url filter hack for symlinkked plugins? The problem on those register_*_hook() seems to be in plugin_basename().

Alternative for plugins_url filter that converts plugin realpath to WP plugin path is naturally to make all symlinked plugins to create their URL differently, e.g. using plugin WPMU_PLUGIN_URL and WP_PLUGIN_URL.

#10 @scribu
13 years ago

Well, anything that uses plugin_basename(__FILE__) won't work properly and can't be fixed by hooking into 'plugins_url'.

Changing each plugin to pass to plugin_basename() what it would expect is not a good solution.

Hence the proposed filter, which would allow to handle this from a single point.

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

#11 @scribu
13 years ago

  • Keywords commit removed

An alternative solution would be to allow plugins from multiple directories, like we do for themes.

@augustash
13 years ago

Fix for correctly extracting the local directory and filename regardless if the file comes from a symlink or not

#12 @augustash
13 years ago

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

the above patch has been tested in our local and production environments. The patch is really simple as all that's needed is to use the basename() and dirname() functions to extract the local directory and filename from the passed in file.

dirname doc = http://www.php.net/manual/en/function.dirname.php

basename doc = http://www.php.net/manual/en/function.basename.php

or use pathinfo to get all the information = http://www.php.net/manual/en/function.pathinfo.php

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

#13 @scribu
13 years ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

The ticket will be closed automatically when a fix is commited.

What's up with WP_fix_for_plugin_basename_to_allow_symlinks_2011-07-28.2.patch?

#14 follow-up: @yincrash
13 years ago

this patch is incorrect. it only works two levels deep. my issue is that that a lot of the jetpack modules are acting screwy when symlinked, and their JS and CSS files are 3/4 levels deep in the plugins folder.

#15 @yincrash
13 years ago

  • Cc yincrash added

#16 in reply to: ↑ 14 @augustash
13 years ago

To clarify, the WP_fix_for_plugin_basename_to_allow_symlinks_2011-07-28.2.patch is a patch to the WP_fix_for_plugin_basename_to_allow_symlinks_2011-07-28.patch so apply the WP_fix_for_plugin_basename_to_allow_symlinks_2011-07-28.patch first then the WP_fix_for_plugin_basename_to_allow_symlinks_2011-07-28.2.patch.

Replying to yincrash:

this patch is incorrect. it only works two levels deep. my issue is that that a lot of the jetpack modules are acting screwy when symlinked, and their JS and CSS files are 3/4 levels deep in the plugins folder.

This patch is not incorrect. You're welcome to contribute to it if you have found scenarios that have nested paths, but I'll bet those scenarios are easily solved by using relative paths to the directory determined by this plugin basename patch.

#17 @paulschreiber
13 years ago

  • Cc paulschreiber@… added

I keep getting bit by this and end up patching individual plugins (Raw HTML Pro, Advanced Custom Fields, More Fields, Post Page Association, etc.) They end up generating bad paths to CSS and JS — the <link> and <script> tags have the filesystem path embedded in them.

It would be nice if WordPress' plugins_url() did the right thing out of the box.

There are other complaints about symlinked plugins:

I urged you to seriously consider fixing this for 3.3.x or 3.4.

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

#18 @boonebgorges
13 years ago

  • Cc boonebgorges@… added

#19 @DJPaul
13 years ago

  • Cc djpaul@… added

#20 @CoenJacobs
12 years ago

  • Cc coenjacobs@… added

#21 @lkraav
12 years ago

  • Cc lkraav added

#22 @gandhiano
12 years ago

Had the same issue - symlinks are defintely important if one is managing a complex WP farm.

Applying the patches did solve the issue, so I propose that these are included upstream.

#23 @scribu
12 years ago

  • Owner scribu deleted
  • Status changed from reopened to assigned

#24 @simonwheatley
12 years ago

  • Cc simon@… added

#25 @leewillis77
12 years ago

  • Cc junk@… added

#26 @juzzin
12 years ago

  • Cc juzzin added

@mitchoyoshitaka
12 years ago

Alternative approach, which caches symlink targets and replaces them

#27 @mitchoyoshitaka
12 years ago

  • Cc mitcho@… added

Patching up plugin_basename() (or allowing for that patching) isn't the only issue here; symlinked files will show up in the plugins listing in wp-admin, but symlinked directories are not. I just attached a patch which also touches get_plugins() to support this.

My fix to plugin_basename() is also a little more robust, but perhaps more overhead than wanted. I hope this patch is helpful for others.

In the mean time, if this functionality should come as a plugin, scribu's pre_plugin_basename isn't the only hook needed; a filter in get_plugins() will also be necessary. If these filters are added, I'd be happy to release a "Symlink Plugins Support" plugin.

#28 @kchrist
12 years ago

  • Cc kenn@… added

#29 @rhertzog
12 years ago

  • Cc hertzog@… added

FTR, this feature is also needed for the setup picked by the official Debian package where we wanted to have packaged plugins in and manually installed plugins kept in two different directories (which is not really possible, so we put symlink to packaged plugins in the directory which is controlled by the local administrator). It would thus be nice to see this issue fixed.

See http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=686228 for a report of this bug on the Debian side.

#30 @kwisatz
12 years ago

  • Cc kwisatz added

Weirdly enough, while augustash's patch works for me (debian wp 3.4.2 package), mitchoyoshitaka's (included in there) does not.

#31 @kchrist
12 years ago

This is also an issue for WordPress sites deployed via Capistrano.

Capistrano keeps previous deployments of the site so you can easily roll back changes. So the actual filesystem path to the site might be:

/home/username/web/example.com/releases/20121018101521/public

But it's symlinked as:

/home/username/web/example.com/current/public

The latter is the path in my web server config. The current directory is a symlink to the latest deployment, which is actually the datestamped releases/20121018101521 directory.

As a result, the entire site is in a symlinked directory. This isn't an issue for most plugins but it's come up more than once in my experience running a handful of sites this way.

#32 follow-up: @scribu
12 years ago

#22802 was marked as a duplicate.

#33 @toscho
12 years ago

  • Cc info@… added

#34 in reply to: ↑ 32 @MikeSchinkel
12 years ago

Replying to scribu:

#22802 was marked as a duplicate.

#22802 is related, but it's not a duplicate.

#35 @scribu
12 years ago

Since #22802 was reopened anyway, let's keep this ticket focused on a solution that would enable symlinked plugins without plugin authors having to do anything special.

#36 @kawauso
12 years ago

  • Cc kawauso added

#37 @synapticism
12 years ago

  • Cc synapticism added

#38 @markus.magnuson
12 years ago

  • Cc markus.magnuson@… added

#39 follow-up: @markus.magnuson
12 years ago

I resolved all my plugin problems caused by symlinks in the wp-content path by using the realpath() function in my wp-config.php. I simply replace this:

define('WP_CONTENT_DIR',$_SERVER['DOCUMENT_ROOT'] . '/wp-content');

With this:

define('WP_CONTENT_DIR', realpath($_SERVER['DOCUMENT_ROOT'] . '/wp-content'));

Thought that might help anyone ending up in this ticket when troubleshooting.

#40 @cklosows
12 years ago

  • Cc cklosowski@… added

#41 @MikeSchinkel
12 years ago

  • Cc mike@… added
  • Keywords dev-feedback added
  • Version set to trunk

I've uploaded a path with another approach. The patch hooks 'pre_update_option_active_plugins' so anytime 'active_plugins' is saved it finds which active plugins have been symlinked and saves an array to a 'symlinked_plugins' key of wp_options. The array has an element for each of the symlinked plugins with the array key being realpath( $virtual_plugin_file ) and the value being virtual_plugin_file, i.e.

update_options( 'symlinked_plugins', array(
   '/Users/user/Plugins/my-plugin-1/my-plugin-1.php' =>  
   '/Users/user/Sites/test/wp-content/plugins/my-plugin-1/my-plugin-1.php',
 
   '/Users/user/Plugins/my-plugin-2/my-plugin-2.php' => 
   '/Users/user/Sites/test/wp-content/plugins/my-plugin-2/my-plugin-2.php',
));

Then plugins_url() simply checks to see if the value of the $plugin parameter -- which is typically passed in as __FILE__ -- is a key in the array returned by get_option( 'symlinked_plugins' ) and if yes it assigns $plugin with that array element's value.

I think that's all it takes. I've been circling this problem for a while so maybe this is all we need to enable plugins_url() to work correctly with symlinked plugins. But I might have missed a requirement and if so will need others to point out what I've missed.

Also please consider this a starting point for discussion; I don't expect that it's completely ready for production, but crossing fingers it might be.

@MikeSchinkel
12 years ago

Patch to enable plugin_url() to support symlinked plugins.

#42 follow-up: @scribu
12 years ago

Re symlinked_plugins.diff: the check you added to plugin_url() should be moved to plugin_basename(), since that's the more general function (and also the one that doesn't have a filter).

#43 in reply to: ↑ 42 @MikeSchinkel
12 years ago

Replying to scribu:

Re symlinked_plugins.diff: the check you added to plugin_url() should be moved to plugin_basename(), since that's the more general function (and also the one that doesn't have a filter).

Originally I didn't put in plugin_basename() because it looked like that would not have worked but I just tested and you are correct, it works. I also tested it with a symlinked plugin where the virtual directory was different than the real directory and it still worked correctly, i.e. my-plugin-dev/my-plugin.php vs. my-plugin/my-plugin.php:

Virtual Path => /Users/my-user/Site/my-site/wp-content/plugins/my-plugin-dev/my-plugin.php 
Real Path    => /Users/my-user/Plugins/my-plugin/my-plugin.php 

I also added an enhancement that omits the 'symlinked_plugins' option if there are no symlinked plugins so that the options table won't won't be polluted with that entry when there are no plugins that have been symlinked.

Attached is a new patch with the check moved to plugins_url().

Last edited 12 years ago by MikeSchinkel (previous) (diff)

@MikeSchinkel
12 years ago

2nd attempt at patch to enable plugin_url() to support symlinked plugins.

#44 @anointed
12 years ago

  • Cc imtiedup@… added

#45 @johnnyb
12 years ago

  • Cc johnbeales@… added

#46 @beaver6813
12 years ago

  • Cc beaver6813 added

#47 @lingfish
12 years ago

  • Cc jason@… added

#48 @SergeyBiryukov
12 years ago

  • Version trunk deleted

#49 @ddean
11 years ago

  • Cc david@… added

#50 @anointed
11 years ago

This has been working pretty well over the past few months.

Is there any chance of this making it into 3.6 so that I can update WordPress without fear of loosing all these code changes?

It has made a HUGE difference for me on the organization of my server files and makes maintaining dozens of sites on a single server so much easier. I no longer have to worry about any clients getting lazy and not updating their plugins and potentially leading to a security breach.

Has anyone at all come up with a problem that would cause this not to work properly in time for 3.6?

*If for some reason it can't be done in time, is it possible to turn this into an 'mu' plugin in the meantime?

#51 @luv4wp
11 years ago

Looks like MikeSchinkels code works for me.

Agree with everyone else here, please add this to the core. Putting it into 3.6 would be great so we don't have to wait 6 months.

@MikeSchinkel
11 years ago

pre-plugin-basename.diff

#52 follow-up: @MikeSchinkel
11 years ago

If adding my patch to the core is too much for 3.6, can you please consider adding a 'pre_plugin_basename' hook that would allow us to create an mu-plugin to do the rest? I've attached a patch for that, above.

#53 in reply to: ↑ 52 @anointed
11 years ago

Replying to MikeSchinkel:

If adding my patch to the core is too much for 3.6, can you please consider adding a 'pre_plugin_basename' hook that would allow us to create an mu-plugin to do the rest? I've attached a patch for that, above.

+1 if we can't have a core patch, this would work just fine for now!

#54 follow-up: @SergeyBiryukov
11 years ago

16953.diff could go in before the beta, but we don't typically add new hooks after feature freeze.

#55 in reply to: ↑ 54 @MikeSchinkel
11 years ago

Replying to SergeyBiryukov:

16953.diff could go in before the beta, but we don't typically add new hooks after feature freeze.

Given how this ticket has been languishing for 2 years with lots of interest and having come full circle, it sure would be nice. :)

#56 @corvannoorloos
11 years ago

  • Cc cor@… added

#57 in reply to: ↑ 39 ; follow-up: @aubreypwd
11 years ago

I can confirm that the below solution worked for me in using:

define('WP_PLUGIN_DIR', realpath('/home/152858/domains/example.com/html/all/wp-content/plugins') );
define('WP_PLUGIN_URL', 'http://example.com/all/wp-content/plugins');
define('WP_THEMES_DIR', realpath('/home/152858/domains/example.com/html/all/wp-content/themes') );
define('WP_THEMES_URL', 'http://example.com/all/wp-content/themes');
define('WP_CONTENT_DIR', realpath('/home/152858/domains/example.com/html/all/wp-content') );
define('WP_CONTENT_URL', 'http://example.com/all/wp-content');

Replying to markus.magnuson:

I resolved all my plugin problems caused by symlinks in the wp-content path by using the realpath() function in my wp-config.php. I simply replace this:

define('WP_CONTENT_DIR',$_SERVER['DOCUMENT_ROOT'] . '/wp-content');

With this:

define('WP_CONTENT_DIR', realpath($_SERVER['DOCUMENT_ROOT'] . '/wp-content'));

Thought that might help anyone ending up in this ticket when troubleshooting.

Version 0, edited 11 years ago by aubreypwd (next)

#58 @JeromeC
11 years ago

  • Cc JeromeC added

#59 @francescolaffi
11 years ago

  • Cc francesco.laffi@… added

#60 @rodrigosprimo
11 years ago

  • Cc rodrigosprimo@… added

#61 @andyadams
11 years ago

  • Cc andyadamscp@… added

#62 @andkrup
11 years ago

  • Cc andkrup added

#63 @tollmanz
11 years ago

  • Cc tollmanz@… added

#64 in reply to: ↑ 57 @mikezielonka
11 years ago

Worked for me as well.

Replying to aubreypwd:

I can confirm that the below solution worked for me in using:

define('WP_PLUGIN_DIR', realpath('/home/152858/domains/example.com/html/all/wp-content/plugins') );
define('WP_PLUGIN_URL', 'http://example.com/all/wp-content/plugins');
define('WP_THEMES_DIR', realpath('/home/152858/domains/example.com/html/all/wp-content/themes') );
define('WP_THEMES_URL', 'http://example.com/all/wp-content/themes');
define('WP_CONTENT_DIR', realpath('/home/152858/domains/example.com/html/all/wp-content') );
define('WP_CONTENT_URL', 'http://example.com/all/wp-content');

Replying to markus.magnuson:

I resolved all my plugin problems caused by symlinks in the wp-content path by using the realpath() function in my wp-config.php. I simply replace this:

define('WP_CONTENT_DIR',$_SERVER['DOCUMENT_ROOT'] . '/wp-content');

With this:

define('WP_CONTENT_DIR', realpath($_SERVER['DOCUMENT_ROOT'] . '/wp-content'));

Thought that might help anyone ending up in this ticket when troubleshooting.

@nacin
11 years ago

#65 follow-up: @nacin
11 years ago

I didn't see until now there was a consensus on a simple filter. My question: What's more useful? pre_plugin_basename, or one just before the return? I'd think the final filter would be more useful, since it smooths out the processing?

#66 in reply to: ↑ 65 ; follow-up: @MikeSchinkel
11 years ago

Replying to nacin:

My question: What's more useful? pre_plugin_basename, or one just before the return? I'd think the final filter would be more useful, since it smooths out the processing?

Yes, given that you are passing in both a post-processed value and a pre-processed value a 'plugin_basename' would definitely be more generally useful and also still meet the need stated on comment 52 of this thread.

Last edited 11 years ago by MikeSchinkel (previous) (diff)

#67 in reply to: ↑ 66 @MikeSchinkel
11 years ago

Replying to MikeSchinkel:

Replying to nacin:

My question: What's more useful? pre_plugin_basename, or one just before the return? I'd think the final filter would be more useful, since it smooths out the processing?

Yes, given that you are passing in both a post-processed value and a pre-processed value a 'plugin_basename' would definitely be more generally useful and also still meet the need stated on comment 52 of this thread.

UPDATE: Except, the hook would have to duplicate the logic in lines 567-574 in 16953.2.diff​ when changing the path, so when the goal is to meet the need stated on comment 52 a 'pre_plugin_basename' would be a better choice because it would limit the potential for error on the part of the plugin author.

#68 @leoj3n
11 years ago

  • Cc leoj3n@… added

#69 @bungeshea
11 years ago

  • Cc info@… added

#70 @F J Kaiser
11 years ago

Generally I'm 100% against what @MikeSchinkel proposes: Syncing one option set with another one. To elaborte the why: We already got that problem in other places, so let's please not add another one. Example: Parent/Child relationships are stored inside get_option( "{$taxonomy}_children" );. Bring that out of sync per accident and you're screwed. A bug that's buried inside a DB option is harder to trace back. And so you already know that you then need will something like _get_term_hierarchy( $tax ); to fix syncing in case.

To stick with Mikes example from #22802 and his directory structure:

/home/myacct/mysite1/  <-- Site #1 goes here
/home/myacct/mysite2/  <-- Site #2 goes here
/home/myacct/plugins/  <-- All the shared plugins go here

# So when you need a URL that looks like this:
http://mysite1.com/wp-content/plugins/myplugin/js/myajax.js

# You instead get this URL because of the symlink: 
http://mysite1.com/wp-content/plugins/home/myacct/plugins/myplugin/js/myajax.js

Wouldn't it be easier to read out the full path and check where exactly you are? Doing this on the fly won't be easier than any other approach, but over all easier to debug. And PHP already got stuff like getcwd() that we can use alongside the directory constants and ABSPATH to compare real paths and symlinked paths.

#71 @rmccue
11 years ago

Discussed on IRC with nacin, best path forward looks to be creating a path cache.

I know exactly how to write the patch, but haven't the time yet. Patches welcome if anyone does.

The approach I'd take is to create a new global (ick, I know, but it can be edited on plugins_loaded) containing a map of dirname( $plugin ) => dirname( realpath( $plugin ) ) in wp-settings.php, then changing plugin_basename to use it. It's simple and it's fairly fast on load (realpath is already hit in include, so the stat cache should fix that). Bonus points for rolling the filter in as well.

#72 @jdgrimes
11 years ago

  • Cc jdg@… added

#73 @volcanicpixels
11 years ago

  • Cc chatfielddaniel@… added

A very inelegant solution, but it is working for me:

https://github.com/danielchatfield/wordpress-symlink-fixer

Can either be a regular plugin or a must-use plugin (some plugins will still not work unless this is a must-use plugin, but mine do).

@rmccue
11 years ago

First pass at using realpath to find symlinks

#74 @rmccue
11 years ago

  • Keywords needs-testing added
  • Owner set to rmccue

Attached patch creates a realpath cache of sorts to match real directories up with their original version. Also introduces a wp_normalize_path() function, since there's a lot of duplicate code in core doing exactly that (I only converted plugin_basename while I was there, but there's plenty more places ripe for patching that in a separate ticket).

Tested locally (Windows system with junction points) and it works. I'd like to have a few more people test it though to make sure.

The only concern I have here is performance, which is why it only stores differing paths (plugins are loaded once, but plugin_basename is called many times). Calling realpath shouldn't be a concern, since it's called on the filename itself which should push it into the stat cache, and is just repeating the realpath call that happens in include anyway.

@rmccue
11 years ago

Again, with a filter as well

#75 @rmccue
11 years ago

Added a filter at the end, same format as nacin's. (Missing hook documentation at the moment though. Added in .4.2.diff, although I gaffed the naming right up.)

Last edited 11 years ago by rmccue (previous) (diff)

@rmccue
11 years ago

Once more with more pizazzHHHH documentation

@rmccue
11 years ago

Once more with more pizazzHHHH documentation and correct variable names

#76 @mindctrl
11 years ago

  • Cc public@… added

#77 @DrewAPicture
11 years ago

@rmccue — 16953.4.2.diff:

  • Short description needs a period
  • Wrap the long description at the comma
  • Periods at the end of the @param and @return lines

:-)

@rmccue
11 years ago

Where you get all them documentation standards

#79 @kchrist
11 years ago

  • Cc kenn@… removed

@rmccue
11 years ago

Check on activation as well

#80 @rmccue
11 years ago

  • Keywords needs-testing removed

16953.7.diff now checks on activation as well, and splits it out into a separate function. This should cover every case in core, I believe.

#81 @westonruter
11 years ago

  • Cc weston@… added

#82 follow-up: @kucrut
11 years ago

  • Cc kucrut.dz@… added

#83 in reply to: ↑ 82 ; follow-up: @MikeSchinkel
11 years ago

Replying to rmccue:

16953.7.diff now checks on activation as well, and splits it out into a separate function. This should cover every case in core, I believe.

Great work. However you missed uninstall_plugin(). Patch 16953.8.diff​ attached.

Last edited 11 years ago by MikeSchinkel (previous) (diff)

@MikeSchinkel
11 years ago

Added wp_register_plugin_realpath() in uninstall_plugin() to 16953.7.diff

#84 in reply to: ↑ 83 @rmccue
11 years ago

  • Milestone changed from Future Release to 3.8

Replying to MikeSchinkel:

Great work. However you missed uninstall_plugin(). Patch 16953.8.diff​ attached.

Thanks for catching that. This is in dd32's hands for committing to the 3.8 branch.

#85 @anonymized_3102866
11 years ago

Have been working on a solution for a private project, but the latest patch seems to cover it all. Works great for me, love to have it in a future release.

#86 @engelen
11 years ago

  • Cc engelen added

#87 @jonathanbardo
11 years ago

  • Cc bardo.jonathan@… added

#88 @raamdev
11 years ago

  • Cc raam@… added

#89 @rmccue
11 years ago

  • Keywords 3.8-early added

Forgot to add the early keyword here.

#90 @dalesaurus
11 years ago

  • Cc dale.liszka@… added

#91 @dd32
11 years ago

  • Keywords 3.9-early added; 3.8-early removed
  • Milestone changed from 3.8 to Future Release

Bumping now that we're in beta. Had this have been a longer cycle I'd have gladly put this in for testing, so lets see come 3.9.

Did anyone investigate if this would cause any performance issues if plugins_url() is called often?

#92 @cgtm
11 years ago

  • Cc cgtm added

@jdgrimes
11 years ago

Add network plugin support in multisite

#93 follow-up: @jdgrimes
11 years ago

16953.9.diff refreshes 16953.8.diff and adds support for network-active plugins in multisite. I moved the initialization of $GLOBALS['wp_plugin_paths'] to up above where the mu-plugins are loaded. Basically it needs to be initialized as an array before there are any calls to plugin_basename(), otherwise that will give an "Invalid argument supplied for foreach" error. Maybe there is a better place to initialize that?

#94 in reply to: ↑ 93 ; follow-up: @rmccue
11 years ago

Replying to jdgrimes:

16953.9.diff refreshes 16953.8.diff and adds support for network-active plugins in multisite. I moved the initialization of $GLOBALS['wp_plugin_paths'] to up above where the mu-plugins are loaded. Basically it needs to be initialized as an array before there are any calls to plugin_basename(), otherwise that will give an "Invalid argument supplied for foreach" error. Maybe there is a better place to initialize that?

I didn't think plugin_basename() could safely be used in mu-plugins? If so, +1 on your patch then.

#95 in reply to: ↑ 94 @jdgrimes
11 years ago

Replying to rmccue:

I didn't think plugin_basename() could safely be used in mu-plugins? If so, +1 on your patch then.

It wouldn't be used by the mu-plugin for its own path, but I know for example that sometimes mu-plugins are just used to load a regular plugin, which will likely be using plugin_basename().

#96 follow-up: @tollmanz
11 years ago

I have tested 16953.9.diff and it works well for a situation where I was bit by this "bug". My situation involves a Capistrano-like "releases" directory that is symlinked to a web root. Within the WordPress installation, the wp-content directory has been moved. I experienced problems with WordPress SEO and this fixed it.

Now that 3.9 is moving along, can this be further tested and integrated. I think that we should aim to build a suite of unit tests that address the different scenarios. This will likely be tricky given that to perform the tests, the environment needs to be configured in a special way. It's probably worth-while to increase the confidence that the chosen solution is backwards compatible, yet solves the issues.

#97 @cliffseal
11 years ago

16953.9.diff also worked great in an environment I was testing. It's got a WordPress Skeleton-esque setup with core and wp-content in the same directory; inside wp-content: mu-plugins, plugins, and themes are all symlinked. Installing the patch fixed all the 404's, and I've yet to run into any issues.

#98 in reply to: ↑ 96 ; follow-up: @nacin
11 years ago

Replying to tollmanz:

I have tested 16953.9.diff and it works well for a situation where I was bit by this "bug". My situation involves a Capistrano-like "releases" directory that is symlinked to a web root. Within the WordPress installation, the wp-content directory has been moved.

This isn't really Capistrano-specific, or really related at all, as Capistrano is symlinked from the web root. At least where I have seen this, wp-content is not typically then symlinked from elsewhere.


I found a few missing wp_register_plugin_realpath() calls. Specifically around plugin sandboxing and also for mu-plugin inclusion. Network plugins are in the normal plugins directory. mu-plugins are separate; they are indeed supported by plugin_basename() and are represented by the WPMU_PLUGIN_DIR directory.

Trying this out sans-filter.

#99 @nacin
11 years ago

  • Milestone changed from Future Release to 3.9

#100 @nacin
11 years ago

In 27158:

Detect and handle symlinking of plugins in plugin_basename().

props rmccue, MikeSchinkel, jdgrimes.
see #16953.

#101 in reply to: ↑ 98 ; follow-up: @tollmanz
11 years ago

Replying to nacin:

This isn't really Capistrano-specific, or really related at all, as Capistrano is symlinked from the web root. At least where I have seen this, wp-content is not typically then symlinked from elsewhere.

I think my situation was similar to what you described. "wp-content" is not symlinked elsewhere. It follows a typical Capistrano setup.

Thanks for landing this. I'll test my scenario and report back to you.

#102 in reply to: ↑ 101 @rmccue
11 years ago

Replying to tollmanz:

I think my situation was similar to what you described. "wp-content" is not symlinked elsewhere. It follows a typical Capistrano setup.

This is more about handling if a plugin itself is symlinked; e.g. wp-content/plugins/myplugin/ -> /var/www/common/myplugin/

Moving wp-content is separate (and usually done by defining WP_CONTENT_DIR/WP_CONTENT_URL) and probably won't be affected here.

Also, <3 nacin.

#103 @TobiasBg
11 years ago

This. is. great! Thanks!

Just briefly tested this, and it seems that load_plugin_textdomain() (and related functions) might also need some treatment.

#104 @nacin
11 years ago

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

load_plugin_textdomain() is fine; it's just for inclusion. The only point of this is for plugin_basename() accuracy.

#105 follow-ups: @iandunn
11 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I upgraded a 3.8.1 install to 3.9-beta1 via WP-CLI today and got about a dozen of these:

PHP Warning: Invalid argument supplied for foreach() in wp-includes/plugin.php on line 620

Attaching 16953.10.diff to check if it's an array before trying to iterate over it.

@iandunn
11 years ago

Check if $wp_plugin_paths is an array

#106 in reply to: ↑ 105 @nacin
11 years ago

Replying to iandunn:

I upgraded a 3.8.1 install to 3.9-beta1 via WP-CLI today and got about a dozen of these:

PHP Warning: Invalid argument supplied for foreach() in wp-includes/plugin.php on line 620

Attaching 16953.10.diff to check if it's an array before trying to iterate over it.

I am trying and failing to determine when this would occur.

#107 in reply to: ↑ 105 @SergeyBiryukov
11 years ago

Replying to iandunn:

I upgraded a 3.8.1 install to 3.9-beta1 via WP-CLI today and got about a dozen of these:

PHP Warning: Invalid argument supplied for foreach() in wp-includes/plugin.php on line 620

Confirmed (with WP-CLI). Only got two of them though, and only on subsequent upgrades to the current nightly build, not on the initial one.

#108 @rmccue
11 years ago

I suspect this could be a WP-CLI problem. Relevant PR over there is 1019.

Only got two of them though, and only on subsequent upgrades to the current nightly build, not on the initial one.

My guess: on first load, wp_register_plugin_realpath doesn't exist (or WP-CLI doesn't realise it does), so the plugin paths never get loaded. On second load, the files have been updated. Still not sure how that would occur, but something along those lines could explain it.

Last edited 11 years ago by rmccue (previous) (diff)

#109 @rmccue
11 years ago

Actually, looks like the above PR already fixes it; original ticket is 1015. We could add the check for WP-CLI specifically, but not sure if we should.

#110 @nacin
11 years ago

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

So: updating WP-CLI fixes things, and technically nothing is broken (you just get a warning out of it). With that I'm fine with calling this fixed again.

#111 @SergeyBiryukov
11 years ago

FWIW, I tested with WP-CLI 0.14.1, freshly downloaded from http://wp-cli.org/.

Looks like it includes the commit mentioned in comment:108, but I still got two warnings.

#112 @scribu
11 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I have a symlink that looks like this:

    wp-content/mu-plugins/p2p-debug -> wp-content/plugins/posts-to-posts/debug.php

$wp_plugin_paths ends up looking like this:

[
  'wp-content/mu-plugins/' => 'wp-content/plugins/posts-to-posts'
]

This breaks all calls to plugins_url() inside the Posts 2 Posts plugin, but I'm assuming that most plugins don't have helper files like that.

What worries me is that all symlinks inside mu-plugins end up pointing to the directory of the first symlink. That doesn't seem sane.

So, I propose we remove the wp_register_plugin_realpath( $mu_plugin ); line.

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

#113 @rmccue
11 years ago

Agreed with scribu; mu-plugins can't be handled the same way (since they're all in the same directory).

Makes me wonder if the same can occur if you have e.g. plugins/hello.php -> /usr/hello.php without a directory?

@jdgrimes
11 years ago

Resolve MU-plugin and single file plugin symlinks

#114 @jdgrimes
11 years ago

Replying to rmccue:

Makes me wonder if the same can occur if you have e.g. plugins/hello.php -> /usr/hello.php without a directory?

I've tested and can confirm this is the case.

mu-symlinks.diff resolves symlinked MU plugins or single file plugins. I'm not sure if that's necessary as far as plugins_url(), but if it doesn't cause problems it might be good for consistency.

#115 follow-up: @nacin
11 years ago

It looks like mu-symlinks.diff would need to normalize paths on both sides of the conditional.

I'm open to suggestions as to what to do here. Do we resolve symlinks for single files, or only for full directories? I'm surprised it took us this long to notice. What's the best path forward? We'll have no choice but to revert the whole thing for 3.9 unless there is a plan of action on how to proceed.

#116 in reply to: ↑ 115 @jdgrimes
11 years ago

Replying to nacin:

It looks like mu-symlinks.diff would need to normalize paths on both sides of the conditional.

The paths are normalized on 645-646 in the patch.

I'm open to suggestions as to what to do here. Do we resolve symlinks for single files, or only for full directories?

As I said above, IMO, we should resolve symlinks for single files for consistency. Though it may not be useful for plugins_url(), it would standardize the behavior of plugin_basename() for all symlinks.

I'm surprised it took us this long to notice. What's the best path forward? We'll have no choice but to revert the whole thing for 3.9 unless there is a plan of action on how to proceed.

I'd really hate to see this get reverted now. I think the reason we took so long to notice was that it really isn't that important in terms of plugins_url(). We could skip them for now and add them in later if needed. But I don't see any reason not to go ahead and resolve them. In the rare situation where a single-file plugin might need to use plguins_url() relative to itself it can't.

#117 @nacin
11 years ago

  • Keywords needs-patch added; has-patch dev-feedback 3.9-early removed

I'd really hate to see this get reverted now.

I agree, but all features come with a maintenance cost. I've discussed this before, quite possibly in the context of this ticket. If WordPress core is to do something here, then we are committing to maintain it in the future. Right now, it's not even clear if we can maintain it in the present. In this case, the feature hasn't even shipped, so I have little qualms of pulling it out if we can't figure out how to best move forward here. For more on incorporating features, read this essay from 2004.

I don't have the time to do an in-depth investigation on this to be able to articulate the exact rationale for fixing this one way or the other. Someone else will need to do that and articulate exactly what needs to be done for 3.9. Otherwise it needs to be removed. There are 116 comments on this ticket and 52 people watching it. If this is a shallow bug, then I imagine this should not be difficult.

I'm reverting this on Wednesday, April 9th if we don't have a fix. This isn't meant to be vindictive; this just isn't my priority; rather, it's meant to be motivating. If it belongs in core, then it needs to become at least one other person's priority. If you want this feature, then fight for it in the form of fixing it. Thanks. :-)

#118 @rmccue
11 years ago

Looking into this now.

I'm in favour of the ignore-single-files solution. If we ignore all mu-plugins (since they're all single file; setups with mu-plugins in subfolders can call wp_register_plugin_realpath in their custom loaders) and single-file plugins, we should be OK here. You can still symlink either of these, but any references to plugin_url will break; I don't forsee this being a huge problem, since single-file plugins with external resources are _doing_it_wrong and should use a directory.

@rmccue
11 years ago

Ignore mu-plugins and single-file plugins

#119 @rmccue
11 years ago

Added patch that uses scribu's change to remove mu-plugins registration, and also ignore files living in WP_PLUGIN_DIR and WPMU_PLUGIN_DIR from being registered.

wp_plugin_register_realpath now returns a boolean that indicates whether the path could be registered, for those of you using it in your own plugins.

The following now work correctly:

  • mu-plugins/y.php -> plugins/x/y.php
  • plugins/y.php -> plugins/x/y.php

Confirmed that the following work:

  • plugins/x/ -> /arbitrary/path/x/
  • plugins/ -> plugins-x/
  • plugins/ -> plugins-x/ and plugins-x/y/ -> /arbitrary/path/y

Any other paths we need to handle?

#120 @rmccue
11 years ago

Also worth mentioning, if you use a pattern like mu-plugins/x/x.php with a mu-plugins/loader.php file, you should add the following to your loader file:

$plugins = array(
	'my-mu-plugin/my-mu-plugin.php',
);
foreach ( $plugins as $plugin ) {
	$path = dirname( __FILE__ ) . '/' . $plugin;

	// Add this line to ensure mu-plugins subdirectories can be symlinked
	wp_register_plugin_realpath( $path );

	include $path;
}

#121 @rmccue
11 years ago

New patch ensures we're always comparing normalised paths; props nacin.

@rmccue
11 years ago

Ensure we normalise base plugin directory paths

#122 @jdgrimes
11 years ago

16953.no-single-file.2.diff looks good, but it should update wp_register_plugin_realpath()'s docblock with a @return tag.

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.


11 years ago

#124 @nacin
11 years ago

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

In 27999:

Don't try to resolve symlinks for single-file plugins. plugins_url() should not be used in this context anyway.

props rmccue.
fixes #16953.

#125 @nacin
11 years ago

Thanks rmccue & co!

#126 @DrewAPicture
11 years ago

In 28011:

Add a missing @since 3.9.0 to the PHPDoc for wp_register_plugin_realpath().

See #16953, #27700.

#127 @rmccue
11 years ago

Thanks for the docs correction from Drew and nacin; sorry I missed that!

Note: See TracTickets for help on using tickets.