Problem/Motivation
User wanted to patch D7 regarding the bootstrap process
Proposed resolution
It was decided to not apply the patch, but instead put information into the documentation for drupal_bootstrap()
Remaining tasks
Add documentation to http://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/drupal_bootstrap/7
API changes
Original report by [coolestdude1]
// Text of original report here.
I already have a fix for this and a follow up comment will supply a patch.
Basically to understand what is going on from D6 to D7.....
Drupal changed the way that files are included now to be more dynamic (this is perfectly fine and is actually great) problem is they only declare that variable in root level documents such as index.php and install.php etc.
When you are loading the bootstrap process from a file that is not one of those you will get a php fatal error on all bootstrapping that requires that variable.
Drupal processes bootstrapping of higher level phases by iterating over all previous phases until the specified level of bootstrapping is attained (this is from documentation).
Now I get that one way would be to include that in your 'other' file but that should not be common place we can fix this in core rather easily and painlessly without affecting other modules.
First off in my patch you will see that bootstrap.inc now has the variable definition in the right spot to be global to all of drupal. And the rest of the patch deals with the root level documents.
This patch makes sense to add to drupal as we are not redefining it in multiple places, so if I am in the wrong here please explain, I would be more than happy to further explain myself here, but for the most part it seems pretty strait forward.
NOTE: I do not know how to patch install.php, update.php those two files get real tricky real quick so maybe a supplement patch will have to be added for them. Otherwise you might get a duplicate definition warning.
Comment | File | Size | Author |
---|---|---|---|
#36 | bootstrap_doc-1436272-36.patch | 1.82 KB | Albert Volkman |
#33 | bootstrap_doc-1436272-33.patch | 1.85 KB | Albert Volkman |
#33 | interdiff.txt | 1.36 KB | Albert Volkman |
#30 | bootstrap_doc-1436272-30.patch | 1.85 KB | Albert Volkman |
#30 | interdiff.txt | 1.04 KB | Albert Volkman |
Comments
Comment #1
coolestdude1 CreditAttribution: coolestdude1 commentedHere is the patch file
Comment #2
tstoecklerPlease read the definition of the issue queue priority levels: http://drupal.org/node/45111
This is by no means critical.
The workflow for external scripts to bootstrap Drupal is:
Or something like that.
Please describe the problem you have with that.
Is the only problem the duplication of the definition of the constant in index.php, update.php, etc.? That doesn't seem to be much of a problem, I think.
Comment #3
tstoecklerCrosspost, so setting to "needs review". For me personally, this is a "won't fix", though. I think the code flow described in #2 is quite nice, because it allows other scripts to integrate with Drupal regardless of where Drupal is relative is to the script, simply by adapting DRUPAL_ROOT.
Comment #4
coolestdude1 CreditAttribution: coolestdude1 commentedYup you are right about the priority, I guess what I mean by critical is mainly that this is an easy fix. But yeah my bad sorries.
My point in changing this is a couple of reasons
-Drupal should be as simple as possible to extend so as to promote community enhancements and contribs
-Moving the definition of DRUPAL_ROOT reduces code in other files making them easier to read/understand
-This workflow was undocumented in drupal_bootstrap
-It makes bootstrapping simpler
-Do we really want external devs changing DRUPAL_ROOT?
-Since this variable is for DRUPAL_ROOT drupal can easily create it and other devs will be non the wiser
Comment #5
tstoecklerNo harm taken. It is just that marking issues critical can stop development because of the issue queue thresholds: http://drupal.org/node/1201874
Regarding changing DRUPAL_ROOT, I meant the following: I develop an external script, which integrates with Drupal, then I can do write my script as I did in #2, and then tell people in my installation instructions: "Change the value of DRUPAL_ROOT to where your installation of Drupal is." I think that is better than: "Change the first part of the require_once line to point to the Drupal installation."
In the end it's a matter of taste, though.
See: http://drupal.org/node/259623 for additional considerations.
Comment #6
carlos8f CreditAttribution: carlos8f commentedThe whole purpose of DRUPAL_ROOT is to make it possible for cwd() to be a directory *other* than the Drupal root. The patch has
define('DRUPAL_ROOT', getcwd());
inside bootstrap.inc, which is regressing to D6 thinking and clearly defeating the purpose. Nothing is broken here, unless it's a lack of documentation, which is a separate issue.Comment #7
coolestdude1 CreditAttribution: coolestdude1 commentedcarlos8f: I fail to see your point, if drupal is running it should know where it's working directory is and just create the variable. A dev should not have to change that variable unless we want to make this variable accessible inside of settings.php (blah at this). Also that call is directly from index.php and the other root entry points. In all that variable is declared multiple times to handle the same functionality.
I also want to mention that I may not have the right solution for everyone as I do have to change the entry points to defined paths. So maybe an even better solution is to check to see if the variable exists from the bootstrap perspective and then create it if it doesn't. This will make the only changed file bootstrap.inc in both d7 and d8, but it will also allow for proper bootstrapping to occur without fatals.
tstoeckler: I read over that issue and it all seems really good, I understand doing these globals speeds up the whole process minimally. I also agree it is tastes.
I want to keep this discussion going because it still does not seem right to me.
I am pushing this further down in priority as people seem to have mixed opinions myself included.
Also to note it changes bootstrapping
FROM
TO
Keeping in mind that this is being called from /sites/all/modules/modulefolder
So getcwd does not work here cause it will return the parent directory.
In either case we should make an effort to change the documentation to match whatever the decision of the community is.
Comment #8
carlos8f CreditAttribution: carlos8f commentedI'm not going to play issue ping-pong. But just so we're clear, there is no bug here. And there's no chance of changing the D7 API a year after it was released, so please discuss new stuff in the context of D8.
Comment #9
tstoecklerMoving this to documentation.
I also think DRUPAL_ROOT is fine the way it is, but you are correct that #2 should be documented somewhere. I could swear that I have seen this somewhere before, but a quick search on d.o didn't reveal anything, so.
The API doc for drupal_bootstrap() says:
(http://api.drupal.org/api/drupal/core!includes!bootstrap.inc/function/dr...)
But I think that could be more explicit.
Also not sure, whether this should be online documentation or in-code or both.
Comment #10
tstoecklerFix title.
Comment #11
jhodgdonSo it sounds like the proposed resolution here is to put the information from #2 into the documentation for drupal_bootstrap()? If so, I think this is a fine idea.
Then in addition if someone wants to write up an expanded page on the subject and put it into the online Community Documentation somewhere (somewhere below http://drupal.org/documentation that is), that would also be a fine idea.
Adding this information seems like a good Novice project?
Comment #12
coolestdude1 CreditAttribution: coolestdude1 commentedRoger that, alright I am updating my code to use this style. Thank you all very much, hopefully we can make this more clear for future devs that may want to bootstrap. Other than that the process works really well.
Comment #13
dandaman CreditAttribution: dandaman commentedI've taken the code from #2 and put it into a patch to the documentation of bootstrap.inc.
Comment #14
jhodgdonUmm. I think it should replace the sentence in the previous paragraph that says:
To access the Drupal database from a script without loading anything else, include bootstrap.inc and call drupal_bootstrap(DRUPAL_BOOTSTRAP_DATABASE).
right?
Comment #15
dandaman CreditAttribution: dandaman commentedMaybe you're right. But I'd expect that there would be reasons to do a full bootstrap and also to do a database-only bootstrap in other cases. I'll admit that I've never done either of these; I'm just trying to help out the documentation in the event that I do need them and try to look it up.
Comment #16
tstoecklerMaybe something like this:
I removed the 'fully' from the top sentence and referenced the $phase parameter for the bootstrap levels. There I documented all the levels explicitly. I think that is more obvious than the current documentation. The downside is that the in-code documentation of the constants is largely duplicated, but I think that is worth it. Also got rid of the word 'included' as that could be mistaken by the PHP include statement. I also added a note about the '/path/to/drupal' bit.
I can't be bothered to roll a patch right now, so just as text for now.
Comment #17
jhodgdonI think that this wording (#16) would be good.
Comment #18
colette CreditAttribution: colette commentedHere's a patch with the updated documentation.
Comment #19
Anonymous (not verified) CreditAttribution: Anonymous commentedLooks pretty good! just a few code style issues, but should be pretty easy to fix
This line has a whitespace on it.
Whitespace at the end of this line.
Since this bullet point runs multiple lines, you will want to indent 'data' so that it lines up with the DRUPAL above.
-2 days to next Drupal core point release.
Comment #20
Anonymous (not verified) CreditAttribution: Anonymous commentedHaving looked at this more, I think the original paragraph can be removed and the new paragraph can be changed to:
This paragraph now includes the sentence from the original paragraph:
The other parts of that paragraph seem to have been covered in the new text already.
Comment #21
tstoecklerSince this needs a re-roll anyways:
Since we do "@param" maybe we should call it "$phase parameter" instead of "$phase argument".
Comment #22
colette CreditAttribution: colette commentedHere's the fixed version.
Comment #23
jhodgdonUmmm... Unless I am mistaken, we don't actually want to lose this line, which describes what this function does:
We also don't want to lose this information that describes that earlier phases are automatically run if you request a later phase:
Oh, I see, it's been added to the paragraph about how to bootstrap Drupal. I don't think it belongs with that paragraph. It needs to be where it was, in its own paragraph, with the sample code afterwards.
Also, this line has a typo:
server -> serve... I guess? It doesn't quite make sense to me, maybe could be rewritten to be clearer what it means.
Also, just below the fixed lines in this patch, I see:
Whenever a function is referred to in documentation, put () after the function name.
Comment #24
Anonymous (not verified) CreditAttribution: Anonymous commentedI suggested to colette that she remove this... is it just me, or is this a really awkward way to say this? Maybe something like:
Comment #25
jhodgdonI think the suggested wording in #24 is better. Something like that could go into the @param $phase documentation actually... maybe saying:
That might still be a bit awkward... it's getting kind of late and my brain is not working well, but something similar should work, and I think putting it with the @param documentation makes more sense than having it above.
Comment #26
Albert Volkman CreditAttribution: Albert Volkman commentedI *think* I caught all of the suggestions from above. Interdiff'd against #22.
Comment #27
jhodgdonLooks good! One thing to fix... This is an 8.x patch, so this line isn't correct:
The path should be core/includes.
Comment #28
Albert Volkman CreditAttribution: Albert Volkman commentedYeah, that makes sense.
Comment #29
tstoecklerSadly, I need to mark this "needs work" again. In the meantime our bootstrap constants have changed. Specifically DRUPAL_BOOTSTRAP_LANGUAGE was removed and at the same point (after _PAGE_HEADER and before _FULL) there should now be DRUPAL_BOOTSTRAP_CODE. The line should read:
"DRUPAL_BOOTSTRAP_CODE: Loads code for subsystems and modules."
Comment #30
Albert Volkman CreditAttribution: Albert Volkman commentedInterdiff'd against #26.
Comment #31
tstoecklerAwesome!
Comment #32
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedTypo: boostrap.
Comment #33
Albert Volkman CreditAttribution: Albert Volkman commentedFixed.
Comment #34
tim.plunkettGood catch, @Tor Arne Thune.
Comment #35
jhodgdonThanks all -- a real group effort here! I've committed the patch there to 8.x. The port to 7.x will need to have slightly different bootstrap constants in it, since the function is different there.
Comment #36
Albert Volkman CreditAttribution: Albert Volkman commentedD7 backport.
Comment #37
tstoecklerI verified that the bootstrap constants, including their order and their description are correct. Also the "core" directory is correctly removed from the example code include statement.
Looks good to me.
Comment #38
jhodgdonLooks good to me too -- thanks all! Committed to 7.x.
Comment #40
rmorelli CreditAttribution: rmorelli commentedI must that the trick with
define('DRUPAL_ROOT', '/var/www/drupal');
require_once DRUPAL_ROOT . '/includes/bootstrap.inc';
drupal_bootstrap(DRUPAL_BOOTSTRAP_SESSION);
doesn't always work out of the box.
eg. I am using it with PMapper but immediately after PMapper does complete all its includes, if not something in drupal_bootstrap fools PMapper as it tries to include from root filesystem (linux here) resulting in PMapper not loading.
Comment #40.0
rmorelli CreditAttribution: rmorelli commentedupdating issue summary