WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 months ago

Last modified 3 months ago

#16953 closed enhancement (fixed)

Allow symlinked plugins

Reported by: scribu Owned by: 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 3 years ago.
'pre_plugin_basename' filter
test.zip (2.1 KB) - added by scribu 3 years ago.
WP_fix_for_plugin_basename_to_allow_symlinks_2011-07-28.patch (711 bytes) - added by augustash 3 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 3 years ago.
16953.alternative.diff (2.7 KB) - added by mitchoyoshitaka 2 years ago.
Alternative approach, which caches symlink targets and replaces them
symlinked_plugins.diff (2.6 KB) - added by MikeSchinkel 17 months ago.
Patch to enable plugin_url() to support symlinked plugins.
symlinked_plugins-2.diff (2.3 KB) - added by MikeSchinkel 17 months ago.
2nd attempt at patch to enable plugin_url() to support symlinked plugins.
pre-plugin-basename.diff (673 bytes) - added by MikeSchinkel 15 months ago.
pre-plugin-basename.diff
16953.2.diff (891 bytes) - added by nacin 13 months ago.
16953.3.diff (2.9 KB) - added by rmccue 10 months ago.
First pass at using realpath to find symlinks
16953.4.diff (3.0 KB) - added by rmccue 10 months ago.
Again, with a filter as well
16953.5.diff (3.2 KB) - added by rmccue 10 months ago.
Once more with more pizazzHHHH documentation
16953.4.2.diff (3.2 KB) - added by rmccue 10 months ago.
Once more with more pizazzHHHH documentation and correct variable names
16953.6.diff (3.2 KB) - added by rmccue 10 months ago.
Where you get all them documentation standards
16953.7.diff (4.0 KB) - added by rmccue 10 months ago.
Check on activation as well
16953.8.diff (8.2 KB) - added by MikeSchinkel 9 months ago.
Added wp_register_plugin_realpath() in uninstall_plugin() to 16953.7.diff
16953.9.diff (5.2 KB) - added by jdgrimes 7 months ago.
Add network plugin support in multisite
16953.10.diff (649 bytes) - added by iandunn 4 months ago.
Check if $wp_plugin_paths is an array
no-mu-symlinks.diff (350 bytes) - added by scribu 3 months ago.
mu-symlinks.diff (884 bytes) - added by jdgrimes 3 months ago.
Resolve MU-plugin and single file plugin symlinks
16953.no-single-file.diff (923 bytes) - added by rmccue 3 months ago.
Ignore mu-plugins and single-file plugins
16953.no-single-file.2.diff (1.3 KB) - added by rmccue 3 months ago.
Ensure we normalise base plugin directory paths

Download all attachments as: .zip

Change History (149)

scribu3 years ago

'pre_plugin_basename' filter

comment:1 scribu3 years ago

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

Test files coming up.

scribu3 years ago

comment:2 scribu3 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 3 years ago by scribu (previous) (diff)

comment:3 scribu3 years ago

  • Description modified (diff)

comment:4 scribu3 years ago

  • Description modified (diff)

comment:5 scribu3 years ago

  • Description modified (diff)

comment:6 westi3 years ago

  • Milestone changed from 3.2 to Future Release

Not a 3.2 feature moving to Future Release.

comment:7 scribu3 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.

comment:8 scribu3 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().

comment:9 ciantic3 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.

comment:10 scribu3 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 3 years ago by scribu (previous) (diff)

comment:11 scribu3 years ago

  • Keywords commit removed

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

augustash3 years ago

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

comment:12 augustash3 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 3 years ago by augustash (previous) (diff)

comment:13 scribu3 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?

comment:14 follow-up: yincrash3 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.

comment:15 yincrash3 years ago

  • Cc yincrash added

comment:16 in reply to: ↑ 14 augustash3 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.

comment:17 paulschreiber3 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 3 years ago by paulschreiber (previous) (diff)

comment:18 boonebgorges2 years ago

  • Cc boonebgorges@… added

comment:19 DJPaul2 years ago

  • Cc djpaul@… added

comment:20 CoenJacobs2 years ago

  • Cc coenjacobs@… added

comment:21 lkraav2 years ago

  • Cc lkraav added

comment:22 gandhiano2 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.

comment:23 scribu2 years ago

  • Owner scribu deleted
  • Status changed from reopened to assigned

comment:24 simonwheatley2 years ago

  • Cc simon@… added

comment:25 leewillis772 years ago

  • Cc junk@… added

comment:26 juzzin2 years ago

  • Cc juzzin added

mitchoyoshitaka2 years ago

Alternative approach, which caches symlink targets and replaces them

comment:27 mitchoyoshitaka2 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.

comment:28 kchrist23 months ago

  • Cc kenn@… added

comment:29 rhertzog22 months 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.

comment:30 kwisatz21 months 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.

comment:31 kchrist21 months 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.

comment:32 follow-up: scribu19 months ago

#22802 was marked as a duplicate.

comment:33 toscho19 months ago

  • Cc info@… added

comment:34 in reply to: ↑ 32 MikeSchinkel19 months ago

Replying to scribu:

#22802 was marked as a duplicate.

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

comment:35 scribu19 months 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.

comment:36 kawauso18 months ago

  • Cc kawauso added

comment:37 synapticism18 months ago

  • Cc synapticism added

comment:38 markus.magnuson18 months ago

  • Cc markus.magnuson@… added

comment:39 follow-up: markus.magnuson18 months 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.

comment:40 cklosows17 months ago

  • Cc cklosowski@… added

comment:41 MikeSchinkel17 months 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.

MikeSchinkel17 months ago

Patch to enable plugin_url() to support symlinked plugins.

comment:42 follow-up: scribu17 months 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).

comment:43 in reply to: ↑ 42 MikeSchinkel17 months 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 17 months ago by MikeSchinkel (previous) (diff)

MikeSchinkel17 months ago

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

comment:44 anointed17 months ago

  • Cc imtiedup@… added

comment:45 johnnyb16 months ago

  • Cc johnbeales@… added

comment:46 beaver681316 months ago

  • Cc beaver6813 added

comment:47 lingfish16 months ago

  • Cc jason@… added

comment:48 SergeyBiryukov16 months ago

  • Version trunk deleted

comment:49 ddean15 months ago

  • Cc david@… added

comment:50 anointed15 months 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?

comment:51 luv4wp15 months 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.

MikeSchinkel15 months ago

pre-plugin-basename.diff

comment:52 follow-up: MikeSchinkel15 months 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.

comment:53 in reply to: ↑ 52 anointed15 months 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!

comment:54 follow-up: SergeyBiryukov15 months ago

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

comment:55 in reply to: ↑ 54 MikeSchinkel15 months 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. :)

comment:56 corvannoorloos15 months ago

  • Cc cor@… added

comment:57 in reply to: ↑ 39 ; follow-up: aubreypwd14 months ago

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.


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');

But, I have been using this as of late: https://gist.github.com/aubreypwd/7828624

Last edited 7 months ago by aubreypwd (previous) (diff)

comment:58 JeromeC14 months ago

  • Cc JeromeC added

comment:59 francescolaffi14 months ago

  • Cc francesco.laffi@… added

comment:60 rodrigosprimo14 months ago

  • Cc rodrigosprimo@… added

comment:61 andyadams14 months ago

  • Cc andyadamscp@… added

comment:62 andkrup14 months ago

  • Cc andkrup added

comment:63 tollmanz14 months ago

  • Cc tollmanz@… added

comment:64 in reply to: ↑ 57 mikezielonka14 months 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.

nacin13 months ago

comment:65 follow-up: nacin13 months 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?

comment:66 in reply to: ↑ 65 ; follow-up: MikeSchinkel13 months 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 13 months ago by MikeSchinkel (previous) (diff)

comment:67 in reply to: ↑ 66 MikeSchinkel13 months 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.

comment:68 leoj3n12 months ago

  • Cc leoj3n@… added

comment:69 bungeshea11 months ago

  • Cc info@… added

comment:70 F J Kaiser11 months 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.

comment:71 rmccue11 months 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.

comment:72 jdgrimes10 months ago

  • Cc jdg@… added

comment:73 volcanicpixels10 months 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).

rmccue10 months ago

First pass at using realpath to find symlinks

comment:74 rmccue10 months 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.

rmccue10 months ago

Again, with a filter as well

comment:75 rmccue10 months 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 10 months ago by rmccue (previous) (diff)

rmccue10 months ago

Once more with more pizazzHHHH documentation

rmccue10 months ago

Once more with more pizazzHHHH documentation and correct variable names

comment:76 mindctrl10 months ago

  • Cc public@… added

comment:77 DrewAPicture10 months 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

:-)

rmccue10 months ago

Where you get all them documentation standards

comment:79 kchrist10 months ago

  • Cc kenn@… removed

rmccue10 months ago

Check on activation as well

comment:80 rmccue10 months 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.

comment:81 westonruter10 months ago

  • Cc weston@… added

comment:82 follow-up: kucrut10 months ago

  • Cc kucrut.dz@… added

comment:83 in reply to: ↑ 82 ; follow-up: MikeSchinkel9 months 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 9 months ago by MikeSchinkel (previous) (diff)

MikeSchinkel9 months ago

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

comment:84 in reply to: ↑ 83 rmccue9 months 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.

comment:85 gayadesign9 months 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.

comment:86 engelen9 months ago

  • Cc engelen added

comment:87 jonathanbardo9 months ago

  • Cc bardo.jonathan@… added

comment:88 raamdev9 months ago

  • Cc raam@… added

comment:89 rmccue8 months ago

  • Keywords 3.8-early added

Forgot to add the early keyword here.

comment:90 dalesaurus8 months ago

  • Cc dale.liszka@… added

comment:91 dd328 months 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?

comment:92 cgtm7 months ago

  • Cc cgtm added

jdgrimes7 months ago

Add network plugin support in multisite

comment:93 follow-up: jdgrimes7 months 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?

comment:94 in reply to: ↑ 93 ; follow-up: rmccue7 months 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.

comment:95 in reply to: ↑ 94 jdgrimes7 months 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().

comment:96 follow-up: tollmanz6 months 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.

comment:97 cliffseal5 months 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.

comment:98 in reply to: ↑ 96 ; follow-up: nacin5 months 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.

comment:99 nacin5 months ago

  • Milestone changed from Future Release to 3.9

comment:100 nacin5 months ago

In 27158:

Detect and handle symlinking of plugins in plugin_basename().

props rmccue, MikeSchinkel, jdgrimes.
see #16953.

comment:101 in reply to: ↑ 98 ; follow-up: tollmanz5 months 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.

comment:102 in reply to: ↑ 101 rmccue5 months 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.

comment:103 TobiasBg5 months ago

This. is. great! Thanks!

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

comment:104 nacin4 months 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.

comment:105 follow-ups: iandunn4 months 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.

iandunn4 months ago

Check if $wp_plugin_paths is an array

comment:106 in reply to: ↑ 105 nacin4 months 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.

comment:107 in reply to: ↑ 105 SergeyBiryukov4 months 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.

comment:108 rmccue4 months 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 4 months ago by rmccue (previous) (diff)

comment:109 rmccue4 months 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.

comment:110 nacin4 months 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.

comment:111 SergeyBiryukov4 months 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.

comment:112 scribu3 months 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 3 months ago by scribu (previous) (diff)

scribu3 months ago

comment:113 rmccue3 months 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?

jdgrimes3 months ago

Resolve MU-plugin and single file plugin symlinks

comment:114 jdgrimes3 months 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.

comment:115 follow-up: nacin3 months 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.

comment:116 in reply to: ↑ 115 jdgrimes3 months 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.

comment:117 nacin3 months 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. :-)

comment:118 rmccue3 months 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.

rmccue3 months ago

Ignore mu-plugins and single-file plugins

comment:119 rmccue3 months 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?

comment:120 rmccue3 months 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;
}

comment:121 rmccue3 months ago

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

rmccue3 months ago

Ensure we normalise base plugin directory paths

comment:122 jdgrimes3 months ago

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

comment:123 ircbot3 months ago

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

comment:124 nacin3 months 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.

comment:125 nacin3 months ago

Thanks rmccue & co!

comment:126 DrewAPicture3 months ago

In 28011:

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

See #16953, #27700.

comment:127 rmccue3 months ago

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

Note: See TracTickets for help on using tickets.