Installing Drupal by visiting index.php (rather than install.php) leads to a fatal error when PDO is not enabled

Here is my patch. I don't know if there is a t() like function or an l() like function available during the DRUPAL_BOOTSTRAP_DATABASE phase. If there is, the patch will need to be updated to reflect this.

I had it there first, unfortunately you will get a PDO error before you hit the hook_requirements. As soon as database.inc is included an error will popup. Unless I'm missing something, which is definitely possible since this is my first time working with the bootstrap process. This line is what first calls PDO in includes/database/database.inc:
abstract class DatabaseConnection extends PDO {

This requirement should be added to system_requirements() in modules/system.install, which is checked during the install process. I've confirmed my patch works properly during the install process and is ready for testing.

Nevermind...looking into install_main in install.php...

Maybe better to see if the PDO - if it does exist - is the PDO that does database stuff, someone could use code that happens to contain class PDO that aint the PDO you think it is

New patch that checks to see if the 'PDO' class exists and if the 'PDO' class has the 'query' function right before needing database access. The language for the error screen is my first attempt, so feel free to modify and/or clarify. Sadly, not really much we can do to SimpleTest this, but I studied the install functions and this should be the right spot. Could use some help on some PHP 4 installs to see if this works.

When an installation process starts, the first script requested is index.php, which starts a full bootstrap. During the bootstrap the file includes/database/database.inc is included, resulting in a fatal error in the definition of DatabaseConnection class: "Class 'PDO' not found" (when PDO is unavailable ofcourse). We shoud find other way to check if PDO is available before including the database.inc file.

When an installation process starts, the first script requested is index.php, which starts a full bootstrap. During the bootstrap the file includes/database/database.inc is included, resulting in a fatal error in the definition of DatabaseConnection class: "Class 'PDO' not found" (when PDO is unavailable ofcourse). We shoud find other way to check if PDO is available before including the database.inc file.

subscribing

Hmm... I really don't see any other options besides to make a some kind of check in the database.inc file itself. Attached patch checks in the DRUPAL_BOOTSTRAP_DATABASE in bootstrap.inc and transfers to a PDO error page in install.php

We will require an approach like this #315513: Catch the absence of $databases settings array early. I can provide a patch now that #281446: Add 'status report' for installer requirements has been committed.

The problem is that the database.inc is included in drupal_maintenance_theme() within install_main(), which is called much, much before the call to install_check_requirements.

@Dave Reid: We must avoid this kind of checking in every request since it burns CPU cycles on bootstrap. Checking before the installation process should be enough.

I have created a patch that actually works as expected. It checks the PDO availability in install_check_requirements, before the inclusion of the DB API files. The patch at #315513: Catch the absence of $databases settings array early is a subset of this one, which is required to detects during the DRUPAL_BOOTSTRAP_CONFIGURATION if there is a connection defined, and if not redirect to the installer before continuing to the next bootstrap phase.

These are screenshots of the Installer status report page, the first with PDO not-enabled and the second with PDO enabled.

Tests and feedback are welcome!

Trying the patch in #16, changed the following line in includes/database/database.inc to emulate not having PDO:

abstract class DatabaseConnection extends PDO_INVALID {

Result on request to install.php:
Fatal error: Class 'DatabaseConnection' not found in /home/davereid/Projects/drupal-head/includes/database/mysql/database.inc on line 14

Result on request to index.php:
Fatal error: Class 'PDO_INVALID' not found in /home/davereid/Projects/drupal-head/includes/database/database.inc on line 137

Line 75 of install_main() in install.php require_once DRUPAL_ROOT . '/includes/database/database.inc'; runs before the call to install_check_requirements in line 125.

That would be expected as PDO_INVALID doesn't exist but PDO does. To emulate it you have to disable PDO in php.

If PDO is disabled, it would also be an invalid class, would it not? I'm just worried this is not actually being tested on a truly PDO-disabled PHP install.

So I took the time to actually disable PDO I get the same results as my emulation of renaming extends PDO:

Result on request to install.php (with patch from #16):
Fatal error: Class 'DatabaseConnection' not found in /home/davereid/Projects/drupal-head/includes/database/mysql/database.inc on line 14

Result on request to index.php (with patch from #16):
Fatal error: Class 'PDO' not found in /home/davereid/Projects/drupal-head/includes/database/database.inc on line 137

Please read my logic in #17 for why this patch does not work. There is a call to include 'database.inc' that happens before install_check_requirements in install_main().

Also, if I have PDO enabled, and the patch from #16 applied, I get the same Fatal error: Class 'DatabaseConnection' not found in /home/davereid/Projects/drupal-head/includes/database/mysql/database.inc on line 14 fatal error in install.php, so this is definitely code needs work.

It says in patch #16 that it requires the patch from #315513: Catch the absence of $databases settings array early Did you apply that also?

Patch in #16 contains the changes in the patch from #315513: Catch the absence of $databases settings array early and is the only one that needs to be applied.

@Dave Reid: To test this patch, you should disable PDO extension in the PHP configuration. It is being tested on a truly PDO-disabled PHP install.

Line 75 of install_main() in install.php require_once DRUPAL_ROOT . '/includes/database/database.inc'; runs before the call to install_check_requirements in line 125.

Yes, but it is only included when $verify = install_verify_settings(); return TRUE, i.e., when a DB connection is already setup.

Attached an updated patch, fixing the problems reported in #20. Since I removed the inclusion of the database.inc file from theme.maintenance.inc, is required to include it in any other place, after the requirements checking.

Please test against HEAD.

@dropcube, It's perfectly valid for a user to have their DB information in their settings.php, but not have PDO enabled. That's why I was getting the error. I'm am actually testing this with actual PDO disabled instead of emulating missing PDO. Sorry I keep chiming in, but I truly want to help and get this fixed.

Using patch in #24, valid DB connection settings, but PDO is disabled. Request to index.php or install.php results in:
Fatal error: Class 'PDO' not found in /home/davereid/Projects/drupal-head/includes/database/database.inc on line 137

Another reason it would be perfectly valid to have a valid db connection setting already is if users are upgrading from D6 to D7. Overall, this new direction of checking for PDO in install_check_requirements does the same thing as was attempted in #2-5 (by checking for PDO in system_requirements in system.install) since install_check_requirements checks all the hook_requirements of the profile's modules, which all by default include the system module.

We need to make this a special case that runs very early in install_main(), change install_verify_settings() to return FALSE if PDO is not found, or make a special case install_check_d7_requirements() that runs very early in the install to check for fatal error-making requirements, such as PDO not found.

@Dave Reid: Could you please test with a clean Drupal installation. During an installation process, that started from 0, there is not a valid DB information in settings.php. You are right, when a valid DB configuration is already created, it fails.

To fix this, is required to make some changes in the installer workflow. Also, we can not check requirements in every request, they are just checked one time, before the installation process.

Another reason it would be perfectly valid to have a valid db connection setting already is if users are upgrading from D6 to D7

Yes, the same will happen to PHP 5 -only functions/features, will fail with unrecoverable fatal errors. So I think we should rework the way the requirements are checked befor continuing with this.

What do you guys think about something like #252100: Check if basic requirements are met before installing/upgrading without requiring to upload the full package

Here's another patch built off of #24.
- Moves the PDO check to system_requirements so that it can be checked by install.php and update.php
- If PDO is disabled, the install.php is allowed to get to the requirements page (and system_requirements).
- A graceful PDO error message is also displayed on update.php.

Tested on HEAD with and without PDO enabled.

@catch: Yes!, I just changed my mind about that. Too much to include and require to be an standalone script ;)

@Dave Reid: Even when this patch actually works and fixes the issue, I have been thinking and I would like a better approach:

The first step of the installation/upgrade process should be to run the requirements verification. For this, only bootstrap with the basis to display a maintenance theme, without including the DB layer nor any other layer that is not required.

Once the the server capabilities are checked, the user may continue with the other installer steps as it is now.

This will allow us to include as many basic requiremets as we need, without having to hard code checks before including files.

Here is an screenshot of what I mean:

Issue: #315901: Add server capabilities check to installation/upgrade tasks

@dropcube, I think that would be a good step for the install process, although it's too bad that we'd have to have two different verify requirements-type tasks. The patch I have in #31 seems to work with just the one screen. I should test it on PHP4 though.

@Dave Reid: Have in mind that if there are not server/requirements problems those screens will be skiped.

My idea is to have an initial task, where only the very basic code is loaded. Also, I don't like to do things like if (class_exists('PDO') && !empty($databases)) { include the files }. what about if tomorrow we need to check if class Foo exists before including bar.php file.

The first task may be called "Verify server capabilities" and the other one "Verify profile requirements", or whatever you guys sugest ;)

@dropcube: You make a good point. I'm not sure how it could be accomplished since on PHP4 the install.php fails to run no matter what since there are try-catch statements that fail parsing.

An approach could be to move the install.php functions to other file, and to include that file if the basic requirements are meet, but not sure if this is the Drupal way. Please follow up in this issue: #315901: Add server capabilities check to installation/upgrade tasks

@dropcube: That's a good idea and direction for the server capabilities. I'm still going to continue effort here since this will become a potential issue during D6 to D7 upgrades with fatal PDO errors when running upgrade.php. Here's a revised patch that sticks to the issue at hand.

If and when #315901: Add server capabilities check to installation/upgrade tasks lands, this changes in this patch to the install.php file can be dropped, but the rest will still be important to prevent fatal errors when running update.php.

Actually, I'm just being stubborn. #315901: Add server capabilities check to installation/upgrade tasks would be useful on update.php as well for when Drupal is updated to another major version with different system requirements.

Why are we using class_exists() to check for PDO? That will trip the autoload functionality and depend on parsing order and so forth. Instead, http://us.php.net/extension_loaded is designed for exactly this purpose. :-)

Good call Crell. Revised patch using extension_loaded.

The patch in #42 works fine with and without PDO enabled and with and without $databases setting in settings.php.

When linking to the PHP manual, I suggest using www.php.net rather than us.php.net or other national mirrors.

Code looks OK visually on quick inspection. I will take simpletest's word for it that it passes. #43 is correct about the link, though.

Doc update in #43

FYI: I think the larger size is because my ide removed some spaces at the end of some lines.

Rollin', rollin', rollin' keep them patches rollin' rawhide.

I would suggest the change in theme.maintenance.inc gets documented better. My guess is it's made so database.inc doesn't get called prematurely. But a comment on why it's not included with the rest of them would be helpful.

bot how you scared me so

Is there a good reason not to move all requirement checks to precede the database loading? This "server capabilities / requirements check" duplication seems likely to cause confusion.

#334086: Consistent error - declares PHP doesn't support database type has been marked as duplicate of this issue.

error message:

In your ./sites/default/settings.php file you have configured Drupal to use a mysql server, however your PHP installation currently does not support this database type.

Possible cause: PDO availble, mysql availble, PDO driver not availble.
Result: User Confusion.

can we have a drupal upgrade requirements checker module, so people can check if their server is capable before they upgrade.

Also +1 @dropcube

The first task may be called "Verify server capabilities" and the other one "Verify profile requirements"

as certain profiles may add additional requirements based on the modules that get loaded, also errors in the second part mean drupal can run but encountered simply fixable errors, while errors in the first part are usually unfixable without changing the server configuration.

A D6 version of Upgrade status can/should include such checks for D7.

Also, "availablility" is a typo.

This is still a problem on the current D7 head. On a test setup without PHP PDO enabled, Drupal just provides the following error:

Fatal error: Class 'PDO' not found in /home/kdeez/workspace/playground/drupal/includes/database/database.inc on line 184

Bumping as it doesn't seem there has been any activity on this recently and it's a critical usability issue.

ChrisBryant is right. I found this issue because what is described in #58 happened to me too with the latest D7 tar.

Sorry but is this patch adding another step to the installer?

No, it just makes sure that if PDO is not enabled, that it is caught on the 'requirements' task of the installer and that things don't fail spectacularly because PDO is not enabled (if you try it now, it's a potential massive upgrade #fail for users).

Re-roll with spelling correction and change of URL to requirements page. Is there a reason for install_verify_pdo() instead of just calling extension_loaded('pdo')?

+    $requirements['pdo']['description'] = $t('Drupal requires the PDO library for database access. See the <a href="@system_requirements">drupal system requirements page</a> for more information on how to enable the PDO library.', array('@system_requirements' => 'http://drupal.org/requirements'));

"drupal" should be "Drupal". Or you could leave it out altogether as surely its clear what system requirements page is being referenced.

A larger issue is that I don't see on installation instructions on the requirements page except for a mention in a comment. It seems like, on a quick review, that the requirements page should list PDO as a requirement with a link to an existing or new handbook page that details installation.

patch needs to be reviewed

removed drupal from name.

Requirements page needs and will be updated for D7. I'm just preparing ahead of time.

The description is still far from good, assume one that doesn't know what PDO and API is. Try to start from scratch again, reworking this description without adding to much information.

Maybe we should not even try to explain PDO, and just forward people to on how to fix it.

Maybe we should not even try to explain PDO, and just forward people to on how to fix it.

But isn't that exactly what this does?

keith.smith: Yes it links, but what about the rest of the text? Remember that the audience of this text is the one with a "bad" host, so most likely won't know much about this stuff.

Bojhan - Can you give an example? I think this description does well to tell that there is an issue, what is the issue and where to go to fix the issue.

I like it.

Let's remove See the system requirements page for more information. from the sentence. I don't think we should link to a requirements page on d.o -- better to have the information in your Drupal site. Less indirections is better. Especially because the requirements page on d.o is not exactly user-friendly.

@Dries

It seems to me that linking to the requirements page would be best. As we come up with new ways this error can occur we can document it there instead of waiting for a commit. Especially after D7 is released.

Maybe we should make the requirements page more user-friendly.

What about a specific handbook page on this? And then 1.

Pick the right option

If a user can't get past the PDO error message he will: -
a) Work with the hosting provider
b) Re-configure his hosting environment
c) Hack through the drupal files to get past the error message
d) Wait for Drupal to resolve the issue in a newer version for all the rocket-science in the CMS
e) Dump Drupal and move on with a random CMS that doesn't do "PDO" or whatever it means or matters :)

He'll do the same as if he's on a host that doesn't support PHP 5.2. The only real options are A, B, and E. Proper error messages are there to encourage A and B rather than E. Some people will always do E, though, because they'd rather use their POS web host than Drupal. That's the price we pay for using technology that's less than ten years old.

How about helping come up with better error messages so that we can maximize A and B?

I tried to install drupal 7 today for several hours. I spent them with debugging ... as I run in several errors in the following order:

XAMPP 1.7.0 on Windows ... Apache crashes because it's not working properly with PDO (As I found out later)

I changed to another server with Ubuntu/dapper and PHP 5.1.2 and got these errors: http://drupal.org/node/540034 because drupal 7 needs PHP 5.2 as minimum. (As I found out later, too)

After this I searched in the drupal forum for the requirements. I could not find a simple page for it but several entries for different versions of drupal, lots of questions and different answers. So I wrote a polite support request to which I wanted to upload the file with my error report. The upload failed and my elaborated text to ask for help was vanished. I thought 'This is a sign, not to ask stupid questions' ... and gave up asking.

I then tried to install the latest XAMPP Version 1.7.2 for Windows Vista ... but in this Versions seems to be a bug as I couldn't get php and mysql working together. (I really tried - but only for 20 minutes.)

I then decided to fix my XAMPP 1.7.0 as described here: http://www.apachefriends.org/f/viewtopic.php?f=16&t=32617 (hint: to download latest php version rewrite the path as link is broken)

Now XAMPP works and I get this error: Fatal error: Class 'MergeQuery' not found in D:\http\drupal7\html\includes\database\database.inc on line 721

Well, I'm rather discouraged now (or one could say depressed). It would be really nice to have a simple list for checking the proper requirements or - even better - to have a 'check requirements' page as first step of the installation process.

Whith this post I vote for it - and I hope to spare somebody hours of searching and trying, searching and trying, search ...

@anschinsan: You mean like http://drupal.org/requirements that's easily findable, linked from either the README or INSTALL.txt in your Drupal download and is the #1 Google result for "Drupal requirements"?

Is this actually a support request now, or did varun21 make a mistake at #78?

Yeah that was a bad meta-data change as well as a bad attempt at sarcasm.

This is going to put up some really bad roadblocks for your average Drupal users.
I thought one of the over-arching concepts behind Drupal 7 was to make it easier to use.
At this stage of the game it won't even install on servers that have been happily running Drupal 6.

Just wanted to mention I had this issue come up. I'm not really sure how to get PDO..

Subscribing.

Attached patch re-rolls ten-month old patch from #71 against current head.

This patch includes the remove of node title as field.

<sheepish>Whoops. To clarify #91 having talked to dereine in irc, I turns out I didn't correctly re-checkout head in my patch in #90 so it includes my re-rolled patch from #571654: Revert node titles as fields by mistake.</sheepish> :)

Very minor improvement on #91, with a small addition that eliminates one silent error thrown otherwise:

--- includes/theme.inc	9 Jan 2010 06:35:38 -0000	1.565
+++ includes/theme.inc	9 Jan 2010 12:25:23 -0000
@@ -2266,7 +2266,7 @@ function template_preprocess(&$variables
-  if ($variables['db_is_active'] = db_is_active()  && !defined('MAINTENANCE_MODE')) {
+  if ($variables['db_is_active'] = (!defined('MAINTENANCE_MODE')) && db_is_active()) {

Good job.

(!defined('MAINTENANCE_MODE')) && db_is_active()) {

Those additional parentheses aren't necessary. This isn't Lisp :P

Re #95.

Excellent!!! Thanks so much, folks! :D In addition to this being better for usability, it also fixes an extremely nasty bug if you don't have a settings.php file in place.

Committed to HEAD!

This issue still exists in the alpha release as well as the current dev version. I marked #690122: Install error as a duplicate.

Actually, the issue has been fixed, but only if you install by going directly to install.php. Since the instructions indicate going to the base URL and not install.php people are still seeing a blank page instead of the requirements verification if they don't have PDO enabled.

Confirming that I too just encountered this bug with the Drupal 7.0-alpha1 release (2010-Jan-15) on a completely fresh install, following the instructions in INSTALL.txt.

At step 4 , "point to the base URL of your website", I get:
Fatal error: Class 'PDO' not found in /home/public_html/drupal-7.0-alpha1/includes/database/database.inc on line 186

Ah, finally found the correct issue thread...

There needs to be a better check for actual PDO support or at minimum provide adequate requirement documentation. My first exposure to D7 (the alpha release) was aggravating and a mystery. On my system the PDO library was installed and enabled, but there were no drivers. Specifically, install_verify_pdo() returned fine, but PDO::getAvailableDrivers was returning an empty array. See #698502: Your web server does not appear to support any common database types. for my experience. This is going to happen on any system that modularizes PHP and related libraries into convenient installation packages.

I had to think on this one a little. We need the install script to be able to run from the base URL for a variety of reasons, but mostly it's to accommodate non-English users who have little or no use for the English documentation included with Drupal. Where ever it's possible, we need to minimize dependence on this documentation.

Since install.php doesn't suffer from this wsod issue, we can address this issue by redirecting to install.php at the top of index.php. Then the redirect code can be removed during the installation or upgrade process to prevent it from being called on every page.

class_exists() is pretty fast AFAIK, and you can tell it to skip attempting an autoload as well. Given how many other crazy slow things we do along the critical path, I don't think one extra C-level call is going to hurt us badly enough to notice.

extension_loaded is faster than class_exists, read Zend/zend_builtin_functions.c

And both are insanely fast, yes.

@catch
"We cannot add a step to allow web writable permission to index.php then remove them after install, it's bad enough for settings.php."

If settings.php is treated like that, and bootstrap.inc includes settings.php, and index.php requires bootstrap.inc, what's the difference?

But I agree that "We can't introduce a check for PDO on every page request, so here's a patch for INSTALL.txt" is the right way to go. The patch contains a typo: 'intall' where it should be 'install'.

@Cybergarou

I had to think on this one a little. We need the install script to be able to run from the base URL for a variety of reasons, but mostly it's to accommodate non-English users who have little or no use for the English documentation included with Drupal. Where ever it's possible, we need to minimize dependence on this documentation.

I'll buy that argument when we internationalise drupal.org but not yet. With all the other language barriers around drupal, and software generally, this isn't the one to get stuck on, imo. If we do add a test, the error message will be in english anyway, as translation won't be working.

That just shows how little I've paid attention to what actually happens on each page load. I failed to realize that settings.php always loads.

That makes the solution here pretty simple. Just add a line to settings.php to redirect to install.php and remove the line when drupal_rewrite_settings() is called.

if($_SERVER['SCRIPT_NAME'] != "/install.php") {header( 'Location: /install.php' ) ;}

@Cybergarou, I don't see how that will work since settings.php doesn't even exist yet at that point? Also, your code would have to be modified because we can't assume that install.php is always being used (for example, the command-line installer does not use it).

***

Maybe I'm missing something here, but can't we just fix this by moving some existing code around? Currently, the function that redirects you from index.php to install.php is this one, which lives in includes/database/database.inc:

function _db_check_install_needed() {
  global $databases;
  if (empty($databases) && !drupal_installation_attempted()) {
    include_once DRUPAL_ROOT . '/includes/install.inc';
    install_goto('install.php');
  }
}

As far as I know, this function is already called at least once on every page request. So what if we just moved it out of database.inc, renamed it appropriately, and then made sure that it was called a bit earlier in the bootstrap process (before includes/database/database.inc ever gets loaded)? Any reason that wouldn't work?

[Actually, just realized before submitting this that that's more or less what's being proposed at #315513: Catch the absence of $databases settings array early, which was linked to further above - although the patch in that issue is way out of date. The basic approach there seems to make sense to me.]

Guys, is this a dup of #641408: PHP extensions and PDO functionality should be checked at installation. ?

I have tested it on my (modular) PHP system where I have very fine-grain control over extension .so's (Arch Linux)

At this point, I think it is. (This issue is older, but that one is doing it more cleanly.)

I don't think this is a duplicate, although I can see how it sounds like it based on the issue title. So I'm changing the issue title to match the specific topic that's been discussed since #98. I don't see how the patch at #641408: PHP extensions and PDO functionality should be checked at installation. could help with that since it doesn't change the point in code where the PDO extension is checked for.

As mentioned above, this might very well wind up being a duplicate of #315513: Catch the absence of $databases settings array early though. If an up-to-date patch shows up there that fixes this issue, we can probably close this one.

You're right, I hadn't tested visiting just '/' instead of '/install.php', which is where this issue lies.

I have a suggestion for a new approach. How about we mock PDO when it isn't available and call it a day?

#115 works for me.

BAHAHAHAAHA I love that approach.

I don't get why that one specific dblog test fails. Any ideas??

Also, something to think about: What if a site is installed then migrated to another server that doesn't have PDO? Mocking when we have a 'functional' site may lead to some strange behavior. It's worth a test... That is unless this is the wrong way to go about it anyway ;)

Except that it means we're always defining and parsing and compiling an extra class on every page load, even if it doesn't get added to the symbol table in PHP 99.9% of the time. That's just the way PHP works; the compile overhead is there either way. That's a *really* ugly hack.

I have to ask... If the issue is that we're trying to use the PDO class before the check to determine if the PDO class is available, why aren't we just putting a dead-trivial check higher in the bootstrap phase and being done with it? Heck, add a "check to make sure we don't need to install" bootstrap phase all of its own and do this and any other checks there. (I've not been looking at the past patches in detail; I guess I'm just wondering why it's apparently a harder problem than it seems like it should be.)

In summary:
If a user tries to install drupal without PDO enabled:
- If they go to /, they get an ugly error
- If they go to /install.php, the error is handled

Possible solutions:
- Change the docs to say install from install.php, i.e. don't support/support with degraded experience installing from /, #100
- More adequate requirements documentation, #101
- Add a test to index.php, or some connected file, and rewrite index.php after the test has passed, #103
- Check extension_loaded for every page load, #106
- A mock PDO class, #115

Each is unsatisfactory, being either inadequate or inefficient.

David Rothstein suggests in #110 moving some code from _db_check_install_needed() in database.inc, along the lines of #315513: Catch the absence of $databases settings array early, which makes a lot of sense.

In a similar way, could we not catch this when we test whether settings.php exists in drupal_settings_initialize(), which is called in the first phase of drupal_bootstrap(). If settings.php doesn't exist, it must presumably be an install visit, if so why not check whether PDO exists and if it does not advise the user? This would add no extra code on a normal visit.

+1 subscribing

+1 subscribing

Yes, I have been using (loving) Drupal since 6 was released. I just downloaded drupal-7.0-alpha3.tar.gz (for the first time) onto my Vista PC running PHP 5.2.8 and MySQL 5.1.31.

I thought I'd report back on this usability issue:

- I unpacked the tar and renamed it 'bigpicture'.
- I did not create a database because I wanted to see what message was passed to the user.
- I created a new tab in Firefox and typed 'localhost/bigpicture'
- I got the following message 'Fatal error: Class 'PDO' not found in F:\www\bigpicture\includes\database\database.inc on line 183'.
- I did a search on Drupal and found this node (http://drupal.org/node/299308).
- After reading the posts I then typed 'localhost/bigpicture/install.php' and I got a 9 point checklist. 6 boxes were green but 3 boxes were red.

The explanations and links on these boxes were good and I then enabled extension=php_pdo.dll in the php.ini file.

So I am up and running but I am sure Newbies would not know how to get past 'Fatal error: Class 'PDO' not found...' so we need to look at how to avoid this error message just appearing.

Otherwise my first impressions for D7 are that it looks great.

Dig

Thanks for your first impression feedback, Dig1! :)

I definitely don't like the idea of forcing people to go to /install.php, so it'd be great to come up with a solution here. The mock class feels a bit hackish. willmoy's proposal in #122 looks to be worth investigating.

This patch was worked on at the drupalconsf2010 code sprint by @cha0s, @gordon, and @sean charles

This patch moves the db_check_install_needed function to the bootstrap.inc file and renames it to _drupal_check_install_needed.

This patch also adds a check for the availability of the PDO extension using extension_loaded('pdo') and if it does not exist there is a redirect to the _drupal_check_install_needed function which in turn redirects to the install.php file.

One of the major issues was ensuring the bootstrap was loaded before redirecting to the install.php.

subscribe

Re-roll the patch

Clean up the trailing white spaces.

Minor reroll to fix hunk error. Otherwise this works as advertised removing the pdo class error and can be marked rtbc if the bot gives green

As per marcingy's instructions... :)

Not so fast... :)

This adds a dependancy on a drupal function to the database layer and Crell has been working very hard to avoid calls like that. Can't we simply leave the db code alone and add those three lines to bootstrap.php?

Actually the lines have been moved to the bootstrap as it is not possible to load the databasse.Inc include without PDO being enabled

Gordon

yes, they *moved*, together with the function the are in :)

That is exactly what I meant. Now we are calling a function that is inside bootstrap.inc from database.inc and we are *really* trying to avoid that. This is called a cyclic dependency, because database.inc needs bootstrap.inc and bootstrap.inc needs database.inc (you could also replace the files with subsystems). We have many of these in core, but the database layer is new and Crell tried to avoid these from the beginning as far as possible.

Instead, what I meant is copying those few lines. Code duplication is bad too, I know, but avoiding cyclic dependencies is imho more important.

database.inc already has several places that rely on bootstrap.inc, so this patch wouldn't be adding any new dependencies - and bootstrap.inc is kind of a special case anyway (the point of its existence is that all parts of Drupal are supposed to be able to depend on it). Also, the existing function was never a database-specific function in the first place; it already contained dependencies not only on bootstrap.inc but also on install.inc! Moving it to bootstrap.inc actually leads to better code separation, IMO.

I haven't reviewed the rest of the patch carefully yet, but on that one point at least I don't see why there would be a problem with it.

Yeah, it was my idea to change the naming of the function to reflect the fact that it doesn't actually belong in database.inc (IMO). It has other dependencies and semantically, it means/does more than just database operations.

EDIT: I don't like cyclical dependencies as much as the next architect ;) but, I think when you bootstrap a system it might be okay to bend that rule. If you can think of a better solution, please share.

I can't find the cyclical dependency?

The _drupal_check_install_needed() doesn't depend on database.inc, it is only checking settings.inc to see if the database has been configured.

If there was a cyclical dependency I would 86'ed this approach when we were discussing this change.

I reviewed the patch and I don't think it's correct.... From the above discussion and #315513: Catch the absence of $databases settings array early, I thought the point of refactoring this code and moving the install check out of the database file was so that we could avoid having to do the extension_loaded() check on every page request, so why are we still adding it?

Instead, I think we want to do something like the attached: Take all those duplicate checks out of the database layer entirely, and move it to a single place in the bootstrap layer, right before we start using the database system. This is simpler, enforces better code separation, and is a tiny bit faster also.

Anything wrong with this? I manually tested installation and the D6->D7 update and everything seemed to work fine.

I like the latest patch. Tested it; looks good. The approach is cleaner and the tests pass. Great work!

Dang. This is really nice! Makes the installer flow far easier to grok, and I especially love that it removes more code than it adds. :D

I would like to get at least one more review on this before committing, since there was so much back and forth above. Then, feel free to move back to RTBC and I'll happily commit this.

I have been following the development closely with the guys and have put in a lot of input.

I think this is quite good, and make the installation basically floor less so it will give all new users the best experience when installing and will not plaster errors all over the screen if the user doesn't have PHP set up right.

I do not think this is going to get much better, and I like the fact that it as removed code from the db drivers, which will make developing new drivers a little easier.

Excellent. Thanks, gordon!

Committed to HEAD! :D

Re-marking as closed (fixed) due to Drupal.org reading a different field follow the D7 upgrade.