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

b doesn't support multisite #12

Closed
jlfranklin opened this issue Jan 14, 2017 · 24 comments · Fixed by #37
Closed

b doesn't support multisite #12

jlfranklin opened this issue Jan 14, 2017 · 24 comments · Fixed by #37

Comments

@jlfranklin
Copy link
Member

jlfranklin commented Jan 14, 2017

B has no way to say b --root /var/www/backdrop --url www.example.org to select sites/example.org/settings.php instead of /var/www/backdrop/settings.php.


After some time of silence here, we now have several options:

@jlfranklin's PR: #21
@indigoxela's PR: #35 Deprecated in favour of #37
@BWPanda's PR: #37

jlfranklin added a commit to sd-backdrop/b that referenced this issue Jan 14, 2017
jlfranklin added a commit to sd-backdrop/b that referenced this issue Jan 14, 2017
jlfranklin added a commit to sd-backdrop/b that referenced this issue Jan 15, 2017
jlfranklin added a commit to sd-backdrop/b that referenced this issue Jan 15, 2017
jlfranklin added a commit to sd-backdrop/b that referenced this issue Jan 15, 2017
jlfranklin added a commit to sd-backdrop/b that referenced this issue Oct 12, 2017
@ghost
Copy link

ghost commented Oct 1, 2019

Yes, I get the following messages when running b st from Backdrop's root directory in a multisite installation:

 [!]   BackdropCMS is not installed yet.                
 [x]   Required bootstrap level for status is not ready 
 [o]   Backdrop CMS Installation detected; wooot;       

And the following error when running the same from a multisite directory (e.g. /sites/[SITE]):

require_once(core/includes/bootstrap.inc): failed to open stream: No such file or directory
        /[SNIP]/b/b.php:82
PHP Fatal error:  require_once(): Failed opening required 'core/includes/bootstrap.inc' (include_path='.:/usr/share/php') in /[SNIP]/b/b.php on line 82

EDIT: I get those message before trying this issue's PR... Will report back once I've tested the PR.

@ghost
Copy link

ghost commented Oct 1, 2019

After applying the patch/PR I get the following results from Backdrop's root dir and a site dir respectively:

 [!]   BackdropCMS is not installed yet.                
 [x]   Required bootstrap level for status is not ready 
 [o]   Backdrop CMS Installation detected; wooot;       
 [x]   Required bootstrap level for status is not ready 
 [o]   Backdrop CMS Installation detected; wooot;       

This is certainly an improvement on my previous attempt.

@ghost
Copy link

ghost commented Oct 1, 2019

But I discovered that "Backdrop CMS Installation detected; wooot;" in my site directory is incorrect. There are two issues:

  1. That is being output from a check that the 'BACKDROP_ROOT' constant is set, however it doesn't check whether that root is correct (e.g. by checking for 'index.php', etc.), just that it's set to a/any value
  2. @jlfranklin's PR sets 'BACKDROP_ROOT' to the current directory before checking if it's actually the root directory.

@indigoxela
Copy link
Member

Here's an alternate PR which works for me: #35

No need for a core patch and the change is relatively small. Please test and review.

@indigoxela
Copy link
Member

Some info about how to use it: there's no new parameter in my PR. You either cd to the directory containing your active settings.php or you set it with "--root=".

Examples for a multisite install:
b --root=/path/to/backdrop/sites/multi1 status
or

cd /path/to/backdrop/sites/multi1
b status

@ghost
Copy link

ghost commented Oct 3, 2019

For the last few days I've been working on this issue (sorry to not mention it sooner), and have come up with the following PR.

It does the following:

  • Adds support for a url option which is checked against sites.php (e.g. b --url=panda.id.au)
  • Gets the root directory:
    • If the root option is set, it checks that $root/index.php and $root/core/misc/backdrop.js exist
    • If the root option isn't set, it recursively checks each directory in the filesystem (starting from the current one and working backwards/up) until it finds it (as defined by the presence of the index.php and core/misc/backdrop.js files)
  • Gets the site directory (i.e. the name of the directory within /sites) if applicable:
    • If the url option is set, it returns the matching directory from sites.php
    • If the url option isn't set, it recursively checks each directory in the filesystem (starting from the current one and working backwards/up) until it finds it (as defined by the presence of a settings.php file), making sure it's not also the root directory, and that it's within the root directory)
  • Sets $_SERVER['HTTP_HOST'] to either the root directory, or the site directory (if it exists) (this is what allows a multisite to work properly via conf_path() which is used in the existing backdrop_settings_initialize() function)
  • Defines BACKDROP_SITE for use elsewhere (e.g. when downloading projects)
  • Adds Backdrop site to the status report
  • Fixes the output of Backdrop CMS Installation detected to check for the index.php file
  • Allows projects to be downloaded to a multisite's directory (by settings the download target to BACKDROP_ROOT . '/sites/' . BACKDROP_SITE)
  • Fixes an issue where download targets have an extra / at the end

This PR therefore allows you to run b commands from any directory within Backdrop. If in or below a multisite folder, commands will target the site directory (e.g. /sites/[SITE_NAME]). If in or below Backdrop root (but not a multisite folder), commands will target to root directory (e.g. /modules, etc.).

Also, any combination of using the root and url options works properly. For example:

/var/www/backdrop/core/includes$ b --url=panda.id.au st
 [o]   Backdrop CMS Installation detected; wooot; 

 Backdrop CMS       :  1.13.3             
 Backdrop root      :  /var/www/backdrop
 Backdrop site      :  panda-id-au              
 Database driver    :  mysql              
 Database host      :  localhost          
 etc.

Or:

/home/panda$ b --root=/var/www/backdrop st
 [x]   Current directory and --root don't match. Please change to a directory under --root or remove --root option. 
 [o]   Backdrop CMS Installation detected; wooot;                                                                   

 Backdrop CMS       :  1.13.3
 Backdrop root      :  /var/www/backdrop 
 Database driver    :  mysql             
 Database host      :  127.0.0.1         
 etc.

The "Current directory and --root don't match..." error is because url wasn't specified, therefore it searches the current folder for a site directory. However this isn't supported as the current directory isn't within the root directory. So it still finds Backdrop root, but it won't find the site unless you run it from with Backdrop root.

Feedback, suggestions, complaints welcome 😉 Also lots of testing!

@ghost
Copy link

ghost commented Oct 3, 2019

Updated my PR to use PHP functions for getting parts of the path (no functionality changes). Thanks @indigoxela!

@indigoxela
Copy link
Member

@BWPanda your PR needs some more work, as it is currently conflicting with your latest merges.

@ghost
Copy link

ghost commented Oct 4, 2019

Yeah, thought it might. Thanks.

Also, before I merge this I'm hoping to get some tests happening (#19) and also to hear feedback from @jlfranklin (since it was his issue originally and also his PR)...

@alanmels
Copy link
Member

Coming from #60. Thank you for all the hard work. Any ETA for the merge?

@ghost
Copy link

ghost commented Oct 22, 2019

I have updated the PR to work with the latest changes in b, but in order to write tests for the multisite functionality we need a way to install a multisite from the command line. This is currently blocked by backdrop/backdrop-issues#4151. So I'll commit this without tests, and create a separate issue for multisite tests once that feature is added to core.

@ghost
Copy link

ghost commented Oct 22, 2019

Or maybe I'll just patch Backdrop in the test environment setup...

@indigoxela
Copy link
Member

Or maybe I'll just patch Backdrop in the test environment setup...

That sounds nasty.

Many thanks for your effort to make b a handy tool for Backdrop. What a progress in such a short time!

@indigoxela
Copy link
Member

indigoxela commented Oct 22, 2019

Trying to keep up with the latest changes... ;-)

It seems to me, we need a more robust way for core file inclusion. See my comment in https://github.com/backdrop-contrib/b/pull/37/files
(I hope, that one is still the most recent PR.)

This is how drush seems to do it:
https://github.com/drush-ops/drush/blob/49cee4dc1e6748116d230e11579a872a3c3b9906/lib/Drush/Boot/DrupalBoot.php#L294

@ghost
Copy link

ghost commented Oct 22, 2019

I think it's fine. If you look a few lines up you'll see that we change to the root directory before running those commands.

@indigoxela
Copy link
Member

Hm, I thought it might, but on my testing machine I get:
require_once(core/includes/bootstrap.inc): failed to open stream: No such file or directory

Just to make sure: this is the correct repo and branch? https://github.com/BWPanda/b/tree/issue-12

@ghost
Copy link

ghost commented Oct 22, 2019

@indigoxela Yep, that's the right repo/branch. I posted a comment in the PR for you.

@indigoxela
Copy link
Member

indigoxela commented Oct 23, 2019

I'll answer your question/comment here, so the PR isn't bloated...

Ha! Silly me, I didn't checkout the right branch after doing a fresh clone of your repo. Much better now.

But...
For multisite installs it only works from either within the sites' directory containing the active settings.php or from the main Backdrop directory using "--url".

Using --url from outside doesn't work at all, from inside it's unreliable, because different depending on the current directory.
From e.g. the /files or /modules directory it works fine, from the /core directory there's an exception (Call to undefined function b_backdrop_installed() in /var/www/work/b/b.php:107).

For single site installs it's pretty much the same (inside/outside...)

It would be cool, if this all could get more robust. Ideally more like drush does it. And without php errors and exceptions. ;-)
Personally, I'd start with the include/require paths - to my opinion they shouldn't be hardcoded relative paths relying on chdir().
And probably we need a "--path" option, too. But that could be a follow-up issue.
Silly again, that's the root option... - but that option needs some attention, too. Especially re multisite.

@indigoxela
Copy link
Member

indigoxela commented Oct 23, 2019

The first few require_once in b.php are dangerous. Use __DIR__ to make sure, you include the right file, not any that happens to have the same name. ;-)

Example: require_once __DIR__ . '/includes/common.inc';

This fixes the exception mentioned in my previous comment.

As soon as BACKDROP_ROOT is set in b_init(), use that constant consequently in all require_once for Backdrop files to prevent problems with different paths. For instance in updatedb.b.inc.

Checking if this is an (already) installed Backdrop could get improved.

If neither "--root" nor "--url" were given as option, step up/down the tree to find a usable settings.php, which doesn't contain the initial value. Or a sites.php. Of cource with depth limits.

If "--root" was provided, verify that the settings.php right there is OK (has database settings different from default value).

If "--url" was provided - hmmm - not sure yet... This probably must happen inside an install... step up/down?

@BWPanda what do you think?

@ghost
Copy link

ghost commented Oct 23, 2019

Thanks for the review/feedback @indigoxela! I've updated my PR with your suggested fix for require_once's, please test again and let me know how that goes.

I've also created some additional issues for the other things you mentioned: #83 and #84

Finally, regarding your comments about the root and url options, that should be the way this PR works already... See the last 3 functions here: https://github.com/BWPanda/b/blob/issue-12/includes/filesystem.inc#L299
If it's still not working properly, please respond with examples of what you're trying/seeing and I'll try to reproduce.

@indigoxela
Copy link
Member

First of all: cool, the exception triggered by the wrong file included is gone.

But I'm confused regarding the root option.

Somewhere outside Backdrop, if I try:
/var/www/work/b/b.php --root=/var/www/backdrop/html st
I get:
[x] Current directory and --root don't match. Please change to a directory under --root or remove --root option.

Sure the path doesn't match. That's why I set the option. If I can only use that option from the exact directory, it is pointless anyway, or am I missing something?

The url option from inside a multisite install works fine.

@indigoxela
Copy link
Member

Anyway, extending the functionality could get handled in a follow-up issue.
PR #37 provides multisite support and that's what this issue is about. Mission accomplished. 😏

@ghost
Copy link

ghost commented Oct 24, 2019

Thanks @indigoxela. That message could use some re-wording I guess. The point of it is that if you don't specify a --url then it looks in the current directory for settings.php. However if the current dir isn't within --root, then that means it's found a settings.php file somewhere other than the Backdrop installation you specified (e.g. a separate Backdrop installation, or a Drupal installation).

I'll create an issue to make this wording better.

@ghost ghost closed this as completed in #37 Oct 24, 2019
ghost pushed a commit that referenced this issue Oct 24, 2019
@ghost ghost mentioned this issue Oct 24, 2019
@ghost
Copy link

ghost commented Oct 24, 2019

Tests passing, so committed this PR! Multisite-specific tests can be added in #86.

Special thanks to @indigoxela for the invaluable testing and suggestions, and to @jlfranklin for the initial issue/PR.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants