Skip to:
Content

bbPress.org

Opened 6 years ago

Last modified 5 years ago

#3255 reopened defect (bug)

Wrong constant used for loading scripts and styles

Reported by: studioreforma's profile studioreforma Owned by: johnjamesjacoby's profile johnjamesjacoby
Milestone: 2.7 Priority: normal
Severity: normal Version: 2.5.14
Component: Appearance - Theme Compatibility Keywords: reporter-feedback
Cc:

Description

File includes/core/template-functions.php

The bbp_enqueue_style and bbp_enqueue_script functions use the wrong constant WP_CONTENT_DIR and the wrong content_url() function to prepare the styles/scripts for enqueue. Since the files are in the plugin folder, WP_PLUGIN_DIR and plugins_url() should be used instead.

Many WordPress installations have the plugins folder outside the content folder - WordPress allows this natively and it is a legitimate usage. The current code does not work in such installations.

Change History (9)

#1 @johnjamesjacoby
6 years ago

  • Component changed from General to Appearance - Theme Compatibility
  • Keywords reporter-feedback added
  • Milestone changed from Awaiting Review to 2.6
  • Owner set to johnjamesjacoby
  • Status changed from new to assigned
  • Version set to 2.5.14

In 2.6 this constant was moved to bbp_urlize_enqueueable().

Are you able to test with the latest from trunk and confirm if this approach fixes things?

Related: #1358.

#2 @johnjamesjacoby
6 years ago

My concern with hardcoding WP_PLUGIN_DIR is that rules out having bbPress in the mu-plugins directory, or even a custom directory inside of WP_CONTENT_DIR (which is how I have mine running.)

#3 @johnjamesjacoby
5 years ago

  • Keywords reporter-feedback removed
  • Milestone 2.6 deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

I've researched this a bit more, and it turns out changing this constant is not a good idea.

The bbPress entire template stack relies on functions like bbp_locate_template(), which parse their directories against it for plugins, mu-plugins, and themes. Themes are the most common use case, so restricting it to plugins will break one of the most common implementations and usages (one that is employed for WordPress.org, bbPress.org, and BuddyPress.org together.

Thanks @studioreforma for opening this ticket.

Closing as wontfix.

#4 @studioreforma
5 years ago

I am sorry I have missed the earlier messages - seems notifications did not work.

I don't understand how you think hardcoding WP_PLUGIN_DIR is bad, but at the same time currently it's hardcoding WP_CONTENT_DIR? It's still the same, just that at the moment it's not following the WordPress standards and is using a wrong constant/function – since bbPress is a plugin, it must use the plugin dir. More and more often people move their plugin dir outside the content dir, so they can take advantage of modern deployment and version management strategies and this is absolutely 100% standard WP practice - it is not hacking it.

If you want to allow more options to the users for placing those files you just have to incorporate the proper logic in the function for checking at different locations, so that your code is up to date with the WordPress way of doing things.

#5 @johnjamesjacoby
5 years ago

I don't understand how you think hardcoding WP_PLUGIN_DIR is bad, but at the same time currently it's hardcoding WP_CONTENT_DIR?

I explained why above. What part(s) exactly of how the code works (and needs to work) can I help clarify 🧐? (Edited for tone)

at the moment it's not following the WordPress standards and is using a wrong constant/function

It is using all WordPress standards, and is using the correct constant for the purpose that the code is designed to serve.

This API specifically needs to be flexible enough to support plugins, mu-plugins, themes, and custom directories inside of the /wp-content/ directory. WP_CONTENT_DIR needs to be the base directory all template stack checks are performed against in order to support all 4 possibilities.

If you have a different suggestion that maintains support for all 4 possibilities, I’m happy to discuss it. 👍

More and more often people move their plugin dir outside the content dir

I totally understand the use case, but switching to your suggestion fixes your use case by breaking 3 others, so it isn’t a viable solution.

it is not hacking it

I didn’t say that it was.

If you want to allow more options to the users for placing those files you just have to incorporate the proper logic in the function for checking at different locations, so that your code is up to date with the WordPress way of doing things.

Thank you for the lesson, but I’d prefer you skip it. It reads rudely and condescendingly, and that won’t get either of us anywhere or improve bbPress, which is ultimately both of our goals.

One solution (that I don’t think is a very good one) is to allow the base directory constant to be filtered, so that you can write a plugin to narrow the directory down to what you need, and everyone else can continue using what has worked for them for 6 or 7 years.

The reason I’m not keen on this, is because it makes constants no longer constant up inside the application layer, eliminating the single reason why constants are things - reliability. It may be the only way though.

Last edited 5 years ago by johnjamesjacoby (previous) (diff)

#6 @johnjamesjacoby
5 years ago

  • Keywords reporter-feedback added
  • Milestone set to 2.6.1
  • Resolution wontfix deleted
  • Status changed from closed to reopened

#7 @johnjamesjacoby
5 years ago

  • Milestone changed from 2.6.1 to 2.6.2

Move these to 2.6.2.

#8 @studioreforma
5 years ago

OK, so you want to keep 6-7 year old code working. Unfortunately this keeps getting a bigger obstacle in working with WordPress – keeping long outdated code working prevents incorporating modern coding practices and using things like composer, npm, git, automated deployment, etc

It is of course your choice how to develop your code and I do not mean to be rude. In fact, I just reported a bug I ran into working for a client several months ago. I hardcoded a change to make the site work and told them not to update bbPress until it's resolved. My job with them is long done and now I will just report back to them that it is not going to be fixed.

As for the issue with mu-plugins, there is also a WPMU_PLUGIN_DIR constant, and just like you check in content dir, you can check the plugin dir, mu plugin dir, themes dir etc. Or you can have external themes register their path and url to activate. There are many options and I do not know your codebase to suggest the best one. But complex plugins with themes and extensions like WooCommerce work without any issues when the plugins folder, mu-plugins folder and the themes folder are moved outside the content folder.

#9 @johnjamesjacoby
5 years ago

  • Milestone changed from 2.6.2 to 2.7
Note: See TracTickets for help on using tickets.