Skip to:
Content

Opened 3 years ago

Last modified 20 months ago

#2945 reopened defect (bug)

Don't hard-code the 'buddypress' plugin folder

Reported by: cnorris23 Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 1.6
Component: Core Keywords: needs-patch
Cc: aesqe@…, sakthi31

Description

BP has it's location hard-coded to be in a folder named 'buddypress.' The included path fixes this, and in the process cleans up some white space.

Attachments (4)

2945.001.diff (64.2 KB) - added by cnorris23 3 years ago.
2945.002.diff (13.7 KB) - added by cnorris23 3 years ago.
untrailingslashit
2945.003.patch (2.0 KB) - added by luccame 22 months ago.
1.6 trunk patch attempt :-)
2945.004.diff (2.1 KB) - added by cnorris23 22 months ago.

Download all attachments as: .zip

Change History (32)

cnorris233 years ago

comment:1 cnorris233 years ago

I fail. The last sentence of the description should read, "the included patch..."

comment:2 DJPaul3 years ago

I'm struggling to imagine situations where someone wants to rename the BuddyPress plugin folder other than the security-through-obscurity angle. It's a big patch for little gain. What's your use case for this?

comment:3 cnorris233 years ago

I agree that most won't change, or need to change, the BP folder name. Therefore, I have no use case. The reason I made the patch is that WP way is to not hard-code the plugin folder. I can't remember where I read that at the moment, but it is how WP devs and official WP/Automattic plugins handle plugin paths. According to Determining Plugin and Content Directories, the way BP currently handles this is minimal, but sufficient. I only thought to do this because it was getting difficult to manage numerous SVN checkouts all named 'buddypress' in my SVN client. The majority of the patch, excluding the white space cleanup that resulted on file save, is removing a backslash and/or the 'buddypress' folder name. If you'd like to see it without all the white space cruft, I can do that.

comment:4 DJPaul3 years ago

Re-patch it and trailingslashstripit() on BP_PLUGIN_DIR etc so we don't have to change all of the existing paths (and potentially break plugins), etc

cnorris233 years ago

untrailingslashit

comment:5 cnorris233 years ago

Updated patch with untrailingslashit, and less white space cruft.

comment:6 DJPaul3 years ago

Almost there. What do people think about moving the BP_PLUGIN_DIR and BP_PLUGIN_URL declarations to bp-loader.php so we can use those throughout?

comment:7 DJPaul3 years ago

  • Milestone changed from Awaiting Review to Future Release

comment:8 cnorris233 years ago

  • Severity set to normal

Related #3507

Last edited 3 years ago by cnorris23 (previous) (diff)

comment:9 aesqe3 years ago

  • Cc aesqe@… added

comment:10 DJPaul23 months ago

Since 1.5 (and certainly 1.6), I think this should work if you set the BP_PLUGIN_DIR constant in wp-config.php. Anyone want to test?

comment:11 luccame22 months ago

Hi Paul,
I was just experimenting with custom WP_CONTENT_DIR / WP_PLUGIN_DIR / BP_PLUGIN_DIR constants, got stuck on BP setup and found this ticket.

I'm on trunk and I found 3 spot with hard-coded path in ...bp-core/admin/bp-core-update.php:
line 256
line 574
line 588

once these line were corrected, BP setup was completed successfully and I'm playing right now with it and see no other errors so far.

comment:12 boonebgorges22 months ago

  • Milestone changed from Future Release to 1.6

luccame - Thanks for testing. Would you mind writing a patch for those lines? If not, I may be able to do it in a few days.

luccame22 months ago

1.6 trunk patch attempt :-)

cnorris2322 months ago

comment:13 cnorris2322 months ago

New patch should fix admin/bp-core-update.php. Two things

1) I'd still like to see BP use

define( 'BP_PLUGIN_DIR', plugin_dir_path( __FILE__ ) );

instead of

define( 'BP_PLUGIN_DIR', trailingslashit( WP_PLUGIN_DIR . '/buddypress' ) );

This way, a folder name change can be made, and BP will adjust automagically ;) The global can still be defined for more fine-grained control/trickery.

2) The only other thing I can see that will keep this ticket from being fixed is

add_action( 'in_plugin_update_message-buddypress/bp-loader.php', 'bp_core_update_message' );

Until BP gets that fancy singleton that johnjamesjacoby added to bbPress, it might be good to add something like

function bp_get_global_property( $prop = '' ) {
    if ( empty( $prop ) )
        return false;

    global $bp;

    if ( !isset( $bp->{$prop} ) )
        return false;

    return $bp->{$prop};
}

This could mitigate the add_action issue and would even be helpful in cases like those in the patch where I had to add the $bp global just to get $bp->file for plugins_url(). Of course, we can always more constants ;)

comment:14 DJPaul22 months ago

I understand you weren't suggesting it directly, but changing the buddypress class to be a singleton is beyond the scope of this ticket. There were some concerns about backwards compatibility with such a change, and it's not something we're going to investigate for 1.6.

On topic: a better approach would be to put that add_action into a function hooked to bp_init, and then we can access BP's global for the base path and append 'bp-loader.php' to it. Your suggestion basically just encapsulates the $bp global, and I don't think there's much benefit in swapping out "global $bp" to bp_get_global_property(...) everywhere.

Last edited 22 months ago by DJPaul (previous) (diff)

comment:15 boonebgorges22 months ago

plugin_dir_path() is broken for a couple different kinds of setups. See #3772 and #WP16953.

a better approach would be to put that add_action into a function hooked to bp_init, and then we can access BP's global for the base path....

Agreed. Then we can revisit the issue when we make larger architectural changes.

comment:16 cnorris2322 months ago

@DJPaul agreed on the first paragraph. Second paragraph. Doh! That makes way more sense. That's what I get for working way past bedtime. I'll whip up a patch for that.

@boonebgorges I knew it was broken, which is why I didn't include in the patch. It was more of a statement for future deliciousness :)

comment:17 follow-up: DJPaul22 months ago

Also, there's another one of these in BP_Admin::add_settings_link() that doesn't work in all environments.

comment:18 in reply to: ↑ 17 cnorris2322 months ago

Replying to DJPaul:

Also, there's another one of these in BP_Admin::add_settings_link() that doesn't work in all environments.

Are you referring to plugin_basename()?

comment:19 boonebgorges22 months ago

(In [6113]) In wizard, generate asset URLs using $bp->file rather than '/buddypress/'

By passing $bp->file into the plugins_url() function, we're able to make it
easier to change the buddypress plugin directory name.

See #2945

Props cnorris23

comment:20 boonebgorges22 months ago

  • Milestone changed from 1.6 to Future Release

I'll keep the ticket open for further discussion in the next dev cycle.

comment:21 sakthi3120 months ago

  • Cc sakthi31 added
  • Version changed from 1.5 to 1.6

I'm trying to test this. I renamed buddypress folder into social; created bp-custom.php in placed it in plugins directory with the following content
<?php

define( 'BP_PLUGIN_DIR', trailingslashit( WP_PLUGIN_DIR . '/social' ) );

?>

WP_PLUGIN_DIR is already defined in wp-config.php and is working fine

Restarted apache and launched the site. Nothing shows up, only a blank page. Do I need to do anything different.

I'm running WP 3.4.1, BP 1.6.1 and BBPress 2.1.2 installed through Buddypress

comment:22 cnorris2320 months ago

@sakthi31 try removing trailingslashit(). I believe BP_PLUGIN_DIR is expected without a trailing slash. You can also turn on WP_DEBUG to see what kind of errors you're getting, as it could be a plugin that hard coded the BP plugin directory, rather than using BP_PLUGIN_DIR.

comment:23 sakthi3120 months ago

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

Removing trailingslashit worked. Thanks @cnorris23

Apologies for not trying this simple edit

comment:24 cnorris2320 months ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Not yet fixed. Still need to fix "in_plugin_update_message-buddypress/bp-loader.php", and maybe a few other lingering issues.

comment:25 foxly20 months ago

Just a heads-up that allowing users to install Buddypress to "/my-awesome-folder-name" instead of "/buddypress" will make unit-testing *much* more complicated and potentially cause all sorts of unpredictable problems throughout the Wordpress installation.

If you make this change, the unit test system will have to remap the Buddypress slug after loading the DB image, because IIRC Wordpress tracks activated plugins by their folder name in the plugins folder. We were hoping to avoid this in BP's unit test runner because it means manually creating a DB image on every upgrade, then figuring out the SQL commands needed to map it to the arbitrary folder name. It could also *double* test run time because, realistically, you're going to have to run the entire test panel twice ...once for the standard "/buddypress" folder path and once for the "/my-awesome-folder" path.

Allowing users to rename the Buddypress install folder introduces MASSIVE technical problems while offering ZERO benefit in any practical application.

Some users on this ticket seem to be misinterpreting the WP codex article to mean "users should be able to rename an individual plugin's install folder".

This is NOT what the codex says.

The codex says "users should be able to rename the ROOT plugins folder and install their WP installation to an arbitrary location on the server".

They're completely different things.

Wordpress actually uses a plugin's install folder as a GUID and, amongst other things, checks for updates based on it. You can't just go arbitrarily renaming your plugin folder or you'll get collisions ...for example, there is already another plugin that uses "/social" as its folder name: http://wordpress.org/extend/plugins/social/ ...which is the name the user requesting the change has set his BP installation folder to.

I suspect the next time the author of the "social" plugin pushes an update, an update notice will appear on the user's dashboard; and if they click on it, Wordpress will replace Buddypress with the real "Social" plugin.

In the interest of not causing EPIC debugging problems, I strongly recommend not implementing this change.

-F

comment:26 cnorris2320 months ago

Replying to foxly:
My original reply to this was incredibly long and snarky, so in the interest of the community, I've chosen to only respond directly to the statement directed towards me.

Some users on this ticket seem to be misinterpreting the WP codex article to mean "users should be able to rename an individual plugin's install folder".

This is NOT what the codex says.

The codex says "users should be able to rename the ROOT plugins folder and install their WP installation to an arbitrary location on the server".

They're completely different things.

You're right. They are completely different things. However, if you'd like to call me out, please do so directly, rather than passive aggressively saying "some users," and also make sure that you have thoroughly read/interpreted what was said. To quote myself "According to Determining Plugin and Content Directories, the way BP currently handles this is minimal, but sufficient." As said in the statement, BuddyPress was doing things minimally correct, as it wasn't hard coding the root plugin directory, or the wp-content directory locations. However, I said minimally correct, because to fully comply with, and use, the WP API, plugins should be using plugins_url(), plugin_dir_url(), and plugin_dir_path() (or at least mimicking their functionality), which allows these paths and URLs to be dynamically created using only the __FILE__ constant as a reference. The changes proposed by this ticket, and the changes already committed, do nothing different than what is already being done by the majority of actively maintained plugins in the WP plugin directory (reasonable guess), and what is purported by the WP API itself. This includes such plugins as Akismet, bbPress, and BP Media. How did Akismet and BP Media, who both have unit tests available, account for the possibility of users changing the plugin folder name? Simple. They didn't, nor should they.

comment:27 foxly20 months ago

Dude... when reading my TRAC comments, please don't assume I'm speaking in a condescending or snarky tone.

I approach projects with the attitude "we have a bunch of naughty source code that we need to whip into shape" not "so-and-so wrote this block of code and it SUCKS". When developers start playing the blame-assignment game, teams fall apart very quickly.

What actually happened in this ticket, to paraphrase was:

=======

DJPaul: "Are you sure we need to let users rename the plugin folder?"

cnorris23: "You guys should see this helpful info on the WP-Codex. Link"

DJPaul: "Here's a patch that re-maps BP's folder"

luccame: "Here's an updated version of the patch"

boonebgorges: (acknowledges patch)

... lots more discussion

sakthi31: "Here's another patch"

...lots more discussion

foxly: "Hey guys, are you SURE this is a good idea?"

=====

So cnorris23, I'd say you probably the only person on this ticket that didn't misinterpret the codex, or perhaps not fully consider the ramifications of giving BP a dynamic plugin folder name ...and that's why I said "some people on this ticket" instead of "I think developer X might not fully understand the design intent set forth in the codex" ... :)

What happened on this ticket is NOBODY'S fault. Things like this have happened millions of times before on projects, and will happen millions of times again. They're a by-product of distributed development and whenever you catch one in progress, you need to call it out to stop the dev team burning time.

-F


comment:28 foxly20 months ago

However, @cnorris32, I disagree with your statement that...

"The changes proposed by this ticket, and the changes already committed, do nothing different than what is already being done by the majority of actively maintained plugins in the WP plugin directory"

A comment you posted to this ticket two months ago reads:

"New patch should fix admin/bp-core-update.php. Two things

I'd still like to see BP use

define( 'BP_PLUGIN_DIR', plugin_dir_path( __FILE__ ) );

instead of

define( 'BP_PLUGIN_DIR', trailingslashit( WP_PLUGIN_DIR . '/buddypress' ) );

This way, a folder name change can be made, and BP will adjust automagically ;) The global can still be defined for more fine-grained control/trickery."

In my opinion, it really sounds like your design intent is to allow the site admin to change BP's folder name.

Based your comment from four days ago:

"@sakthi31 try removing trailingslashit(). I believe BP_PLUGIN_DIR is expected without a trailing slash. You can also turn on WP_DEBUG to see what kind of errors you're getting, as it could be a plugin that hard coded the BP plugin directory, rather than using BP_PLUGIN_DIR."

It sounds to me like the BP_PLUGIN_DIR constant is allowing users to modify BP's folder name, which is clearly NOT in compliance with WP design practices.

Here's why:

Take a look at how the WP core handles plugin activation:

http://core.svn.wordpress.org/branches/3.4/wp-includes/plugin.php

On line 560 function plugin_basename($file) turns the realpath of a plugin file that looks like this:

"C://var/dir/dir/wp_plugins_folder/plugin_folder/loader.php"


into something that looks like this:

"plugin_folder/loader.php"

That function gets called by

function register_activation_hook($file, $function)

on line 617 in the same file, which includes the code

add_action('activate_' . $file, $function);

So the first thing that will happen if you move buddypress to an arbitrary folder is that any plugin that listens for Buddypress to activate itself is going to break.

http://core.svn.wordpress.org/branches/3.4/wp-admin/plugin.php

Further along in the activation process, inside

activate_plugin( $plugin, $redirect = '', $network_wide = false, $silent = false )


on lines 547 and 551, Wordpress uses the plugin_basename value as a GUID and writes it to the 'active_plugins' and optionally the 'active_sitewide_plugins' options.

http://core.svn.wordpress.org/branches/3.4/wp-includes/update.php

Then, in function

wp_update_plugins()

on line 136 of update.php, on line 147 the function fetches the unique active plugin slugs using

$active  = get_option( 'active_plugins', array() );

on line 205 it checks the slug against the WP plugin repo to see if the repo has been updated

$raw_response = wp_remote_post('http://api.wordpress.org/plugins/update-check/1.0/', $options);

http://core.svn.wordpress.org/branches/3.4/wp-admin/includes/class-wp-upgrader.php

And if the repo has been updated

class Plugin_Upgrader extends WP_Upgrader {

on line 23 gets called, successively downloading and checking the updated zip file from the repo, then deleting the entire contents of the folder that maps to the plugin_basename and overwriting it with the files downloaded from the repo.

The consequence of this is if the admin renames the "/buddypress" folder to be "/awesome-plugin" and "awesome-plugin" exists on the WP plugin repo, then when the author of "awesome-plugin" updates their plugin, the site admin will get an "upgrade alert" for BuddyPress.

If the site admin clicks on "upgrade plugin", Buddypress will be deleted and replaced with "awesome-plugin". In addition to this, all of Buddypress' database tables will probably be dropped and replaced with awesome-plugin's database tables.

These are only some of the problems that renaming the "/buddypress" folder can cause. Because changing the /buddypress plugin folder name to something other than what Buddypress has been assigned by the WP plugin repo falls outside of the WP plugin specification, there is no limit to the potential problems it can cause in future versions of Wordpress.

=====

Nobody on this ticket should feel bad for requesting or going along with this modification request. On the surface it looks like a relatively innocent change. Only developers with very, very in-depth knowledge of the WP core would catch this one, and only after spending a considerable amount of time researching it.

I recommend we make sure Buddypress follows the base folder path generation methods set forth in the codex at the link provided by @cnorris32; but I recommend we immediately remove any code that implies or assumes the user is able to set a BP_PLUGIN_DIR constant.

The existence of a BP_PLUGIN_DIR constant that is *generated by Buddypress* for the convenience of other developers is, of course, totally OK.

-F

Note: See TracTickets for help on using tickets.