[drupal-devel] [bug] Do not execute functions in include files
Dries
drupal-devel at drupal.org
Tue Jun 21 20:24:50 UTC 2005
Issue status update for http://drupal.org/node/18213
Project: Drupal
Version: cvs
Component: base system
Category: bug reports
Priority: normal
Assigned to: chx
Reported by: killes at www.drop.org
Updated by: Dries
Status: patch
The code comments need updating too; they mention init.inc. The
documentation starts talking about 'levels' and then switches to use
the word 'phases' ... I think we should call it a 'boostrap phase', or
'phase'. Is the check if ($called) return necessary? Who's going to
call that function twice?
Dries
Previous comments:
------------------------------------------------------------------------
March 2, 2005 - 00:09 : killes at www.drop.org
Drupal currently executes functions in include files. This is very
annoying if you want to include files from external scripts such as
update scripts or other scripts that want to use Drupal as a framework.
Offending files are:
database.inc:
db_set_active();
session.inc:
session_set_save_handler("sess_open", "sess_close", "sess_read",
"sess_write", "sess_destroy", "sess_gc");
session_start();
common.inc:
A bunch of stuff at the bottom.
All this should be moved into a start.inc file.
------------------------------------------------------------------------
March 27, 2005 - 08:44 : adrian
I haven't done this recently, but back in 4.4 era I attempted something
similar to this for the install api.
A _lot_ of modifications were required to be able to use (for instance)
the theme api without needing to have a database connection active.
------------------------------------------------------------------------
March 27, 2005 - 08:55 : adrian
It's definately not a simple problem.
For instance.. the db connection needs to be started during bootstrap,
and during normal execution.
I think the better idea is to include a DRUPAL_INIT constant we can
check before executing the function. (or a DRUPAL_NO_INIT .. i'm not
picky).
------------------------------------------------------------------------
April 28, 2005 - 23:53 : killes at www.drop.org
Here's an old patch by me that illustrates what I want to do.
http://lists.drupal.org/archives/drupal-devel/2005-03/txtq23pAcHATw.txt
Instead of putting the stuff into index.php, alternative file names
need to be found.
------------------------------------------------------------------------
May 4, 2005 - 19:36 : chx
Attachment: http://drupal.org/files/issues/boot.patch (6.83 KB)
Well, I have written something to address the issue.
Now you can include everything to your hearts content just include
init.inc as well and do an appropriate drupal_bootstrap.
Alas, ATM database and bootstrap is very interwined. This
interdependency could be obliterated if we'd not use
variable_get('dev_query') in database.*.inc.
cvs diff -u -N -F^f against cvs.drupal.org as CVSROOT failed to include
to include the two new files, so I'll include them as followups.
------------------------------------------------------------------------
May 4, 2005 - 19:37 : chx
Attachment: http://drupal.org/files/issues/boot_0.patch (10.31 KB)
Hm, settings.php changes were ommited.
------------------------------------------------------------------------
May 4, 2005 - 19:37 : chx
Attachment: http://drupal.org/files/issues/init.inc (3.46 KB)
------------------------------------------------------------------------
May 4, 2005 - 19:38 : chx
Attachment: http://drupal.org/files/issues/settings_ini_set.php (850 bytes)
------------------------------------------------------------------------
May 4, 2005 - 19:44 : chx
Attachment: http://drupal.org/files/issues/boot_1.patch (14.49 KB)
Following advice from darix I have now created one patch. Still, I'd
like to know how could I do this with cvs.
------------------------------------------------------------------------
May 4, 2005 - 20:01 : chx
Attachment: http://drupal.org/files/issues/boot_2.patch (14.48 KB)
Steef found that I had $$ in settings.php.
He also asked what's this good for. Several things, but most advantages
come when you use only parts of Drupal. If you want use, say, only our
forms functions then now you can include('common.inc'); without side
effects. If you need the database layer, you do a include('init.inc')
and call drupal_bootstrap('database')
------------------------------------------------------------------------
May 5, 2005 - 23:03 : chx
Attachment: http://drupal.org/files/issues/boot_3.patch (17.98 KB)
The database layer is now totally independent of other parts of Drupal.
------------------------------------------------------------------------
May 15, 2005 - 21:37 : chx
A very stupid bug related to variable init is fixed and I am sync'd to
HEAD, so timer_init('page') is called.
------------------------------------------------------------------------
May 16, 2005 - 23:12 : chx
Attachment: http://drupal.org/files/issues/bootstrap_3.patch (12.36 KB)
------------------------------------------------------------------------
June 4, 2005 - 17:29 : chx
Attachment: http://drupal.org/files/issues/bootstrap_4.patch (12.36 KB)
Fixed drupal_bootstrap('database') -- $phase was 0 , and == FALSE fired.
=== FALSE is used.
------------------------------------------------------------------------
June 4, 2005 - 19:03 : nsk
Some days before I was studying the Drupal source code and it caused me
headaches that it executed code via include calls. I think it is
better design to not execute anything automatically in includes.
------------------------------------------------------------------------
June 4, 2005 - 19:25 : chx
Attachment: http://drupal.org/files/issues/bootstrap_5.patch (15.26 KB)
Added phpdoc and moved drupal_unpack to init.inc thus now session
handler can be used independently.
------------------------------------------------------------------------
June 5, 2005 - 11:13 : Dries
Sorry but the quality of the PHPdoc is not up to standards. Please use
proper punctuation, typography, line-wrapping, etc. It should also
explain when/why to use this function.
I can't help but think this is ugly code. drupal_bootstrap, for
example, can be written more elegantly (not tested):
function drupal_bootstrap($phase) {
$phases = array('database', 'session', 'bootstrap', 'common');
foreach ($phases as $step) {
_drupal_bootstrap($step);
if ($phase == $step) {
break;
}
}
}
instead of:
function drupal_bootstrap($phase) {
static $done = -1;
$phase = array_search($phase, array('database', 'session',
'bootstrap', 'common'));
if ($phase === FALSE) {
return;
}
for (; $done <= $phase; $done++) {
_drupal_bootstrap($done);
}
}
------------------------------------------------------------------------
June 5, 2005 - 18:34 : chx
Attachment: http://drupal.org/files/issues/bootstrap_6.patch (15.56 KB)
Some code simplification in drupal_bootstrap and better PHPdoc. (Thanks
Dries and Steven.)
------------------------------------------------------------------------
June 6, 2005 - 17:19 : moshe weitzman
i did a quick code review and this patch looks quite nice to me. note
that it changes some of the same bits that are changed by my 'ban
hosts' patch. i'm ok with committing this one first, and i will update
accordingly.
------------------------------------------------------------------------
June 6, 2005 - 20:56 : Dries
- Patch breaks update.php. Might want to double-check cron.php and
xmlrpc.php.
- The PHPdoc code wraps at variable width/length. Might want to tidy
up that a little.
------------------------------------------------------------------------
June 7, 2005 - 22:25 : moshe weitzman
in the big case statement in drupal_bootstrap(), perhaps use defined
constants instead of 0, 1, 2 ...
i think the 4 phase bootstrap is perfect.
perhaps move the code in the drupal_bootstrap('all') section to
common.inc under a new function called drupal_bootstrap_all(). that
makes the code a bit tidier and keeps a few lines out of init.inc which
is useful. saves a bit of parsing on requests that don't use the whole
framework. you could do that for the other phases as well although the
benefit is smaller.
------------------------------------------------------------------------
June 7, 2005 - 22:58 : chx
Attachment: http://drupal.org/files/issues/bootstrap_7.patch (20.76 KB)
We were already using named phases (not constants), but I have renamed
them now as discussed with moshe. Moved the code for the last phase
(now called 'full') to common.inc. Simplified index.php -- much simpler
than in previous patch, a bit simpler than pre-patch.
------------------------------------------------------------------------
June 7, 2005 - 23:07 : chx
Attachment: http://drupal.org/files/issues/bootstrap_8.patch (16.34 KB)
Following Moshe's advice to see http://talks.php.net/show/acc_php/33 , I
have added a few dots before the paths.
------------------------------------------------------------------------
June 7, 2005 - 23:34 : Bèr Kessels
I love the idea of this. +1 for the last (#8) patch from me. I must
mention i did not test it thoroughly, hust saw that it works here! The
levelled include looks great to me.
A minor thing , that I thnik would make this even better, is an
additional level for feeds. Feeds need not load all the theme, blocks,
and other stuff, only the nodes (items) in the feeds.
For reference, my last statistics:
Top 30 of 4679 Total URLs
# Hits KBytes URL
1 22042 11.11% 2 0.00% /cron.php
2 9418 4.75% 14742 0.60% /favicon.ico
3 6917 3.49% 56598 2.29% /misc/drupal.css
...
13 3272 1.65% 31790 1.29% /node/feed
So my main feeds are the 13th most referenced URL on my sites. (the
4-12 contain a lot of odd deeplinked images, and off course /node and /
)
------------------------------------------------------------------------
June 8, 2005 - 00:04 : chx
Separating node/feed is not an easy thing, maybe impossible First off,
you need most modules as nodeapi is called during this a theme call is
possible. I do not see what can be omitted from 'full'.
I think this could be a next patch, if we can (which I doubt) an
intermediate between 'page cache' and 'full'.
------------------------------------------------------------------------
June 8, 2005 - 08:55 : Bèr Kessels
I figured it would be hard. So lets ignore my post. I will think of a
way to cache it better inside node.module and taxonomy.module.
------------------------------------------------------------------------
June 13, 2005 - 13:08 : killes at www.drop.org
I read the patch and would like to see it get into core rather sooner
than later.
------------------------------------------------------------------------
June 13, 2005 - 13:21 : stefan nagtegaal
This would make things pretty simple.. When we look at the upcoming
install-api, we only have todo:
<?php
drupal_bootstrap('database');
?>
to make database access possible and ready for usage..
I cannot see anything inside the code itself which looks not right,
but:
<?php
+ header('HTTP/1.0 403 Forbidden');
+ print "Sorry, ". $_SERVER['REMOTE_ADDR']. " has been
banned.";
+ exit();
?>
Shouldn't that be something like:
------------------------------------------------------------------------
June 13, 2005 - 13:46 : gordon
+1, I agree that this needs to go into live sooner, or we are not going
to be able to get it into the next release.
This is going to require a good shake down to make sure that it is all
working.
------------------------------------------------------------------------
June 13, 2005 - 15:50 : chx
Stefan, your comment is cut, but even if it were not, that's a different
issue -- I moved that code intact. So if you have some concerns over the
recent deny patch, please reopen that issue.
------------------------------------------------------------------------
June 21, 2005 - 21:04 : Dries
Patch no longer works.
------------------------------------------------------------------------
June 21, 2005 - 21:28 : killes at www.drop.org
Attachment: http://drupal.org/files/issues/bootstrap_9.patch (16.17 KB)
updated
------------------------------------------------------------------------
June 21, 2005 - 21:42 : chx
Attachment: http://drupal.org/files/issues/bootstrap_10.patch (17.38 KB)
thanks killes. xmlrpc and cron patched.
------------------------------------------------------------------------
June 21, 2005 - 21:45 : Dries
Why do we need a init.inc AND a bootstrap.inc? IMO, these names are
somewhat confusing. Can't we merge init.inc into bootstrap.inc (and
keep it simple)?
------------------------------------------------------------------------
June 21, 2005 - 21:49 : chx
Maybe rename bootstrap to something better? I do not want to merge, on
of the good things in this patch that you are not forced to include
bootstrap (which is 20K) only init.inc (which is only 4K).
------------------------------------------------------------------------
June 21, 2005 - 21:53 : killes at www.drop.org
20K difference are really no reason to not merge them.
------------------------------------------------------------------------
June 21, 2005 - 22:12 : chx
Attachment: http://drupal.org/files/issues/bootstrap_11.patch (13.76 KB)
I hate CVS.
More information about the drupal-devel
mailing list