Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make WP_SITEURL smart by detecting if $WORDPRESS_DIR is set #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mchung
Copy link
Owner

@mchung mchung commented Apr 24, 2014

Happy to see that the buildpack supports subdirectory (mchung/heroku-buildpack-wordpress/issues/23).

This patch instructs Wordpress to detect WORDPRESS_DIR and then dynamically sets WP_SITEURL. Currently, this isn't done, so when a developer sets WORDPRESS_DIR via heroku config:set, the site breaks since WP is still serving content out of the previously configured directory. And since the site breaks, the user can't login to update that variable.

The buildpack README also specifies that WP_CONTENT_DIR and WP_CONTENT_URL must be set, however I didn't observe any problems without them set. Is that doc out of date? Or was I just lucky?

cc: @okko

@okko
Copy link
Collaborator

okko commented Apr 24, 2014

WP_SITEURL: The site URL is not necessarily the same as WordPress. We often have the site at "/" and still WordPress in a subdirectory, for easier local development when buildpack is not used to unpack the WordPress distribution. If they are not the same, I guess WP_CONTENT_DIR and WP_CONTENT_URL then need to be set.

Your patch makes the assumption that the site is within the WORDPRESS_DIR. It may or may not be. :-) That assumption in the default template (this project) is fine to make, just making sure you are aware.

@mchung
Copy link
Owner Author

mchung commented May 1, 2014

WP_SITEURL: My understanding is that this should match the base URL + /path that you want the site to be served out of. If we allow "/foo" then WP_SITEURL should be "http://asdf.com/foo" so that Wordpress knows how to serve static content. I couldn't figured out what WP_CONTENT_DIR/WP_CONTENT_URL tunes in this context.

Yes, the patch does assumes the site is within WORDPRESS_DIR. That seemed reasonable as that's aligned with - what I believe - most people want.

@okko
Copy link
Collaborator

okko commented May 1, 2014

With WP_CONTENT_URL/WP_CONTENT_DIR one can set the location of "wp-content" to be outside WORDPRESS_DIR. So if you want to move WordPress to a subdirectory but not wp-content, then you need them. Most users have them within WORDPRESS_DIR.

Anyway, fully agree that it is a reasonable assumption, go ahead and merge. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants