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

Better webroot structure (web/) #32

Merged
merged 8 commits into from Mar 1, 2014
Merged

Better webroot structure (web/) #32

merged 8 commits into from Mar 1, 2014

Conversation

swalkinshaw
Copy link
Member

Bedrock currently exposes a lot of files to the webroot that shouldn't be there. The README provides some web server configs to block access to these files, but it's still not a proper solution.

This PR moves what's required into a web/ directory including the vendor'd WP source, and the app source itself. Now an Apache/Nginx site should be pointed to /path/to/site/web/ instead of just /path/to/site/

The name web was chosen since it reflects that its the webroot. public is a very common name in frameworks like Symfony, Rails, Laravel etc, but it doesn't directly map to our use case since that's always for static file serving like images, scripts, css etc.

See #10 for more discussion.

@lautreamont3
Copy link

    "installer-paths": {
      "web/app/plugins/{$name}/": ["type:wordpress-plugin"],
      "web/app/mu-plugins/{$name}/": ["type:wordpress-muplugin"],
      "web/app/themes/{$name}/": ["type:wordpress-theme"]
    },

What about app/uploads not in web.

@swalkinshaw
Copy link
Member Author

@lautreamont3 thanks, fixed!

@lautreamont3
Copy link

You did not change installer-paths in composer.json to web/app/....

@swalkinshaw
Copy link
Member Author

@lautreamont3 wow only saw your comment about uploads... actually fixed now.

@lautreamont3
Copy link

I am not shure, because I constantly change my vagrant/apache configuration, but I don't think this is configuration error. So I think you need to add this #ADDITION to config/application.php:

$root_dir .= '/web'; #ADDITION 
define('CONTENT_DIR', '/app');
define('WP_CONTENT_DIR', $root_dir . CONTENT_DIR);
define('WP_CONTENT_URL', WP_HOME . CONTENT_DIR);

This will satisfy WP_CONTENT_DIR and ABSPATH but not WP_CONTENT_URL (change of CONTENT_DIR to /web/app will influence both _DIR and _URL, I am kinda arguing with muself).

BECAUSE YOU ONLY READ END OF MESSAGES I SPLIT THIS TO POINT SEPARATE TEXTS

I have one question my configuration don't work if I leave

require_once('../vendor/autoload.php');

in web/wp-config.php, I must use absolute path or I get error

PHP Fatal error:  require_once(): Failed opening required '../vendor/autoload.php'

am I doing something wrong?

@rslnk
Copy link
Contributor

rslnk commented Jan 23, 2014

I came across the same issues with WP_CONTENT_URL and require_once('../vendor/autoload.php'); while trying to incorporate this PR in my Bedrock configuration.

To solve the first issue with WP_CONTENT_URL, I've added a new CONTENT_ROOT variable:

define('CONTENT_ROOT', '/web');
define('CONTENT_DIR', '/app');
define('WP_CONTENT_DIR', $root_dir . CONTENT_ROOT . CONTENT_DIR);
define('WP_CONTENT_URL', WP_HOME . CONTENT_DIR);

As for the require_once('../vendor/autoload.php'); giving an error, my guess is that this has something to do with require_once function working relative to the path of the wp-config.php file from where it is called.

Adding the full file path to the wp-config.php seems to work:

require_once (__DIR__ . '/../vendor/autoload.php');
require_once(__DIR__ . '/../config/application.php');

Note: __DIR__ constant will work with php 5.3+

Both workarounds worked for me, however, I'm not sure if that would be the most elegant solution. Even though I've developed my first WordPress website by being inspired by the Roots theme a few years ago, I'm still only a beginner to the whole new concept of Modern Wordpress Development Workflow.

@swalkinshaw
Copy link
Member Author

Sorry guys, I've just been doing this PR manually without testing it (not a good idea btw). Just opened the PR a bit early, but you're both right.

@rslnk there's nothing wrong with your solution. Requiring absolute vs relative paths isn't really less elegant. Probably better in the long run and using __DIR__ is completely fine since to use Composer you need > 5.3 anyway so anything Bedrock does can (and should) take advantage of 5.3 features like namespaces too.

@lautreamont3
Copy link

No problemo, with this changes everything works like standard Bedrock.

@rslnk Dont forget ABSPATH.

@rslnk rslnk mentioned this pull request Jan 24, 2014
@swalkinshaw
Copy link
Member Author

@rslnk squashed all your commits into 1 but all the work is still there :)

@lautreamont3
Copy link

Correct .gitignore

# WordPress
wp
.htaccess

to

# WordPress
web/wp
web/.htaccess

@@ -22,8 +22,9 @@
/**
* Custom Content Directory
*/
define('APP_ROOT', $root_dir . '/web');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rename APP_ROOT to WEB_ROOT to avoid confusion with the content dir that is named app, also as said in the pull request description its commonly referred as webroot; maybe also move this constant up near $root_dir definition since this section is about custom content dir, just my 2 cents ;)

anyway good PR 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. Just did those changes since they makes sense.

swalkinshaw added a commit that referenced this pull request Mar 1, 2014
Better webroot structure (web/)
@swalkinshaw swalkinshaw merged commit bc3772d into master Mar 1, 2014
@swalkinshaw swalkinshaw deleted the web-folder branch March 1, 2014 05:52
@4selected
Copy link

What's the best to way to have "htdocs" instead of "web" dir? Do I have to fork your repo and modify it on my own?

@retlehs
Copy link
Sponsor Member

retlehs commented Apr 4, 2014

yep.

@4selected
Copy link

Wow, that's fast. do you think it's good to update the README with the new web path for my fork? And leave you and Scott as authors in the composer file? Sorry for this questions, but I'm new to this :/

@etcook
Copy link
Contributor

etcook commented Apr 4, 2014

You could also use a symlink for htdocs to the web directory if that option is avail to you.

@swalkinshaw
Copy link
Member Author

@studio4s I probably wouldn't worry about updating anything else. Not really important in the long run.

@swalkinshaw
Copy link
Member Author

Running composer create-project studio4s/bedrock studio-test picks up the latest tag which Packagist syncs from GitHub if you setup that hook. Since you haven't tagged a new releases, it's taking the last one which is just normal Roots. You can get around it by running:

composer create-project studio4s/bedrock studio-test dev-master

Specifying dev-master will get the latest version from the master branch.

@4selected
Copy link

@swalkinshaw that you so much Scott!!!

onnimonni pushed a commit to onnimonni/Dokku-bedrock that referenced this pull request Apr 10, 2014
Better webroot structure (web/)
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.

None yet

8 participants