We should add PDO as a system requirement to prevent fatal errors on hosts where it's not compiled into PHP

Files: 
CommentFileSizeAuthor
#126 299308.patch4.12 KBSean Charles
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in 299308_1.patch.
[ View ]
#141 pdo-databases-299308-140.patch6.72 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 20,026 pass(es).
[ View ]
#132 pdo-error.patch3.76 KBmarcingy
PASSED: [[SimpleTest]]: [MySQL] 19,923 pass(es).
[ View ]
#130 0001-Fix-up-the-patch-to-be-in-the-right-format.patch3.96 KBgordon
PASSED: [[SimpleTest]]: [MySQL] 19,944 pass(es).
[ View ]
#129 0001-Fix-up-the-patch-to-be-in-the-right-format.patch3.96 KBgordon
PASSED: [[SimpleTest]]: [MySQL] 19,938 pass(es).
[ View ]
#115 299308.patch959 bytescha0s
PASSED: [[SimpleTest]]: [MySQL] 17,901 pass(es).
[ View ]
#100 install.patch737 bytescatch
PASSED: [[SimpleTest]]: [MySQL] 17,533 pass(es).
[ View ]
#96 pdo-requirement-299308-96.patch5.35 KBbabbage
Passed on all environments.
[ View ]
#93 pdo-requirement-299308-93.patch5.35 KBbabbage
Passed on all environments.
[ View ]
#91 pdo-requirement-299308.patch3.99 KBdawehner
Passed on all environments.
[ View ]
#90 299308-89.patch106.91 KBbabbage
Passed on all environments.
[ View ]
#71 pdo-requirement-299308-5.patch4.77 KBredndahead
Passed: 10671 passes, 0 fails, 0 exceptions
[ View ]
#65 pdo-requirement-299308-4.patch4.79 KBredndahead
Passed: 10671 passes, 0 fails, 0 exceptions
[ View ]
#62 pdo-requirement-299308-3.patch4.8 KBredndahead
Passed: 10671 passes, 0 fails, 0 exceptions
[ View ]
#47 pdo-requirement2.patch4.73 KBredndahead
Failed: Failed to apply patch.
[ View ]
#45 pdo-requirement-D7.patch5.03 KBredndahead
Failed: Failed to apply patch.
[ View ]
#42 pdo-requirement-299308-D7.patch4.69 KBDave Reid
Failed: Failed to install HEAD.
[ View ]
#39 pdo_requirement-5-D7.patch4.78 KBDave Reid
Failed: 7199 passes, 4 fails, 0 exceptions
[ View ]
#34 basic-requirements.jpg76.39 KBdropcube
#34 basic-requirements.jpg76.39 KBdropcube
#31 pdo_requirement-4-D7.patch7.93 KBDave Reid
Failed: Failed to apply patch.
[ View ]
#24 pdo-requirement-299308-24-D7.patch3.08 KBdropcube
Failed: Failed to apply patch.
[ View ]
#16 pdo-requirement-299308-16-D7.patch6.19 KBdropcube
Failed: Failed to apply patch.
[ View ]
#16 pdo-not-enabled.png55.9 KBdropcube
#16 pdo-enabled.png50.19 KBdropcube
#13 pdo_requirement-2-D7.patch2.43 KBDave Reid
Failed: 7241 passes, 4 fails, 0 exceptions
[ View ]
#8 pdo_requirement-D7.patch1.49 KBDave Reid
Failed: 7199 passes, 4 fails, 0 exceptions
[ View ]
#5 299308.patch1.01 KBDave Reid
Failed: 7199 passes, 4 fails, 0 exceptions
[ View ]
#1 pdo_requirement.patch1013 bytesredndahead
Failed: Invalid PHP syntax.
[ View ]

Comments

redndahead’s picture

Status:Active» Needs review
StatusFileSize
new1013 bytes
Failed: Invalid PHP syntax.
[ View ]

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.

catch’s picture

Status:Needs review» Needs work

This needs to be in system.module's hook_requirements, there's no point checking on every page load. We'll also need to document it in the handbook.

redndahead’s picture

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 {

catch’s picture

Hmm. Not sure how this works with the installer partial bootstrap, but assuming the same issue, we could maybe special case it in install.php/install.inc?

Dave Reid’s picture

Assigned:Unassigned» Dave Reid
Status:Needs work» Needs review
StatusFileSize
new1.01 KB
Failed: 7199 passes, 4 fails, 0 exceptions
[ View ]

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.

Dave Reid’s picture

Status:Needs review» Needs work

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

dgtlmoon’s picture

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

Dave Reid’s picture

Status:Needs work» Needs review
StatusFileSize
new1.49 KB
Failed: 7199 passes, 4 fails, 0 exceptions
[ View ]

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.

dropcube’s picture

Component:system.module» install system
Status:Needs review» Needs work

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.

dropcube’s picture

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.

earnie@drupal.org’s picture

subscribing

earnie’s picture

subscribing

Dave Reid’s picture

Status:Needs work» Needs review
StatusFileSize
new2.43 KB
Failed: 7241 passes, 4 fails, 0 exceptions
[ View ]

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

dropcube’s picture

Status:Needs review» Needs work

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.

Dave Reid’s picture

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.

dropcube’s picture

Status:Needs work» Needs review
StatusFileSize
new50.19 KB
new55.9 KB
new6.19 KB
Failed: Failed to apply patch.
[ View ]

@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!

Dave Reid’s picture

Status:Needs review» Needs work

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

<?php
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.

redndahead’s picture

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

redndahead’s picture

Status:Needs work» Needs review
Dave Reid’s picture

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().

Dave Reid’s picture

Status:Needs review» Needs work

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.

redndahead’s picture

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?

Dave Reid’s picture

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.

dropcube’s picture

Status:Needs work» Needs review
StatusFileSize
new3.08 KB
Failed: Failed to apply patch.
[ View ]

@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

<?php
   $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.

Dave Reid’s picture

Status:Needs review» Needs work

@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

Dave Reid’s picture

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.

dropcube’s picture

@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.

catch’s picture

Since the database information changed in D7, people won't be able to simply drop their settings.php into their install - it'll have to be fixed as part of the upgrade process (still pending) - so if they're missing PDO or some other requirement they'll hopefully find out early on there.

dropcube’s picture

Title:Add PDO to system requirements» Add PDO to installation requirements

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.

dropcube’s picture

Dave Reid’s picture

StatusFileSize
new7.93 KB
Failed: Failed to apply patch.
[ View ]

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’s picture

Status:Needs work» Needs review

@dropcube, I think install.php ought to handle fatal errors as quickly as possible - ideally moving the basic system checks before install profile installation. The scout seems like a contrib project to me.

dropcube’s picture

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

dropcube’s picture

StatusFileSize
new76.39 KB
new76.39 KB

@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

Dave Reid’s picture

@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.

dropcube’s picture

@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 ;)

Dave Reid’s picture

@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.

dropcube’s picture

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

Dave Reid’s picture

StatusFileSize
new4.78 KB
Failed: 7199 passes, 4 fails, 0 exceptions
[ View ]

@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.

Dave Reid’s picture

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.

Crell’s picture

Status:Needs review» Needs work

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. :-)

Dave Reid’s picture

Status:Needs work» Needs review
StatusFileSize
new4.69 KB
Failed: Failed to install HEAD.
[ View ]

Good call Crell. Revised patch using extension_loaded.

c960657’s picture

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.

Crell’s picture

Status:Needs review» Needs work

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.

redndahead’s picture

Status:Needs work» Needs review
StatusFileSize
new5.03 KB
Failed: Failed to apply patch.
[ View ]

Doc update in #43

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

Anonymous’s picture

Status:Needs review» Needs work

The last submitted patch failed testing.

redndahead’s picture

Status:Needs work» Needs review
StatusFileSize
new4.73 KB
Failed: Failed to apply patch.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch failed testing.

redndahead’s picture

Status:Needs work» Needs review

bot how you scared me so

cburschka’s picture

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.

alexanderpas’s picture

#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.

catch’s picture

Someone on #drupal had a similar issue to the one linked above - pdo and mysql installed, but not pdo_mysql. Most hosts only mention whether they have php or mysql (and maybe versions if you're lucky) - and a lot of users will only have ever needed to worry about that rather than specific extensions/drivers, so the more help we can give here the better. For a lot of people downloading Drupal 7, these errors will be the first they ever hear about the new requirements, and they're likely to be people on either commercial shared hosting, or on centrally managed servers in an organisation. We've also got the fact that this will affect people installing Drupal 7 on servers where Drupal 6 worked fine.

So if I've got it right, we need to first check if pdo is available, then check if there's a driver available. With either failure, seems like our best bet is to link to http://drupal.org/requirements - could probably start a Drupal 7 sub-page of that for now until we get to Beta ,so it doesn't get too cluttered (it's already a very complicated read).

alexanderpas’s picture

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.

catch’s picture

alexandarpas - you mean for Drupal 5 / 6? - that sounds like a good idea, just needs someone to volunteer for it in contrib.

Note that this patch conflicts with #332303: bootstrap from sqlite - but we need to keep graceful error handling/information whatever happens with that.

Crell’s picture

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

catch’s picture

Status:Needs review» Needs work

Tested the patch by renaming /etc/apache2/conf.d/pdo.ini

It picks up the requirements check fine. A couple of things though - I agree with redndahead that this could use some more inline code comments. Also, the link should go to drupal.org/requirements - and we can link to php.net from there if needs be. For many people, enabling PDO will be 'send an e-mail to your hosting provider' or similar - so we need a nice friendly page for those people to visit when they completely fail to install Drupal on their random ass host.

keith.smith’s picture

Also, "availablility" is a typo.

ChrisBryant’s picture

Issue tags:+installation, +Usability, +pdo

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.

andreiashu’s picture

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

Bojhan’s picture

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

Dave Reid’s picture

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).

redndahead’s picture

StatusFileSize
new4.8 KB
Passed: 10671 passes, 0 fails, 0 exceptions
[ View ]

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')?

keith.smith’s picture

+    $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.

redndahead’s picture

Status:Needs work» Needs review

patch needs to be reviewed

redndahead’s picture

StatusFileSize
new4.79 KB
Passed: 10671 passes, 0 fails, 0 exceptions
[ View ]

removed drupal from name.

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

Bojhan’s picture

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.

keith.smith’s picture

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?

Bojhan’s picture

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.

redndahead’s picture

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.

catch’s picture

How about:

Your server does not have the PHP PDO extension enabled. [link to drupal.org]

The main thing is making sure people know it's a server issue (i.e. they can't just download something called PDO from somewhere to their Drupal directory to make it work). But the bulk of the work should be on the requirements page.

redndahead’s picture

StatusFileSize
new4.77 KB
Passed: 10671 passes, 0 fails, 0 exceptions
[ View ]

I like it.

Dries’s picture

Status:Needs review» Needs work

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.

redndahead’s picture

@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.

catch’s picture

If someone gets this error message, they're not going to be able to get past it without a significant change to their hosting environment - whether that's upgrading/recompiling PHP, modifying php.ini etc. - in most places where PDO isn't already enabled, this is going to require contacting their hosting provider, possibly changing hosting provider. In other words, there's a decent chance they won't be able to finish installing Drupal in one sitting.

We need to be able to take people through the process of understanding what PDO is, why the error might be showing up (is the extension just not enabled or is it completely missing?), and how to get it changed - which eventually could involve instructions for specific hosts (or at leat we can't only assume ssh access to the server). This is a lot of information to convey, and if we do a bad job of it, people are going to end up on google or giving up (and hopefully drupal.org/requirements eventually if we're lucky). Additionally the description for the error is absolutely not the place to put this information since there's going to be more than a couple of sentences.

There's various ways this could be done:

1. Commit the patch as is and make sure the requirements page is massively revamped as part of the d.o redesign (which it has to be anyway for people who get there before installing Drupal).

2. Ship with extra documentation on these major errors which can be viewed without a full Drupal bootstrap.

This could go two ways

a. Linking from the status report to .html files shipped with Drupal (similar to the new help system although wouldn't necessarily be connected unless we also wanted them available after install).

b. Class certain install requirement failures as COMPLETE INSTALL FAIL!11!! and make a new splash screen which looks completely different to the status report - "Sorry, your hosting environment is not configured to support Drupal, here's how to fix it".

c. link to INSTALL.txt and put the extra instructions in there.

d. some other relatively large reworking of this page.

However, these are significant changes to the installer, and are likely to need significant discussion before we can implement them - and they apply equally to incorrect MySQL, PHP versions etc.

fwiw, we already link to http://drupal.org/requirements in the following places in core:

catch@catch-laptop:~/www/7$ grep -r ".org/requirements" *
INSTALL.txt:(http://drupal.org/requirements) in the Drupal handbook.
modules/system/system.install:      $requirements['php_memory_limit']['description'] = $description . ' ' . $t('See the <a href="@url">Drupal requirements</a> for more information.', array('@url' => 'http://drupal.org/requirements'));
UPGRADE.txt:   http://drupal.org/requirements.

At the moment we either have a nasty PHP error as the first thing people see when they try to install Drupal, or an addition to the status report. However I think we need to seriously consider what to do in these situations since even if a small percentage of people run into this issue.

Personally I'd support committing the patch as is, then opening a critical/release blocker issue for the "what to say if someone isn't going to make it through the installer in one sitting". I also agree with redndahead that the /requirements page is more likely to stay up to date over the 2-3 years Drupal 7 will be in production.

Bojhan’s picture

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

earnie’s picture

IMO, the requirements need to be prominent at d.o. and a check box completed that the user understands the requirements before downloading the file. Alright, the check box idea is a bit much but the download page must give text that gives strong warning that the hosting provider must have these installed or installable for you. And if the user has an exception the user is redirected to the download page giving the requirements after and error stating that the environment doesn't meet the requirements.

catch’s picture

There's a related discussion over at #315901: Add server capabilities check to installation/upgrade tasks - although the fixes in this patch probably be enough to un-postpone that issue now.

varun21’s picture

Category:bug» support

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 :)

Crell’s picture

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?

anschinsan’s picture

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 ...

Dave Reid’s picture

@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"?

Paul Natsuo Kishimoto’s picture

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

Dave Reid’s picture

Category:support» bug

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

epruett’s picture

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.

MTecknology’s picture

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

catch’s picture

Issue tags:+beta blocker

Bumping this and adding beta-blocker tag - someone just ran into this again on #drupal. Unless you happen to know that class MergeQuery is missing because PDO is missing, then you're completely stuck, and we want people testing betas (hopefully including shaming hosts who don't have PDO installed), not failing to install them and shaming us for useless error messages.

webchick’s picture

Subscribing.

babbage’s picture

Assigned:Dave Reid» Unassigned
Status:Needs work» Needs review
StatusFileSize
new106.91 KB
Passed on all environments.
[ View ]

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

dawehner’s picture

StatusFileSize
new3.99 KB
Passed on all environments.
[ View ]

This patch includes the remove of node title as field.

babbage’s picture

<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> :)

babbage’s picture

StatusFileSize
new5.35 KB
Passed on all environments.
[ View ]

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()) {
chx’s picture

Status:Needs review» Reviewed & tested by the community

Good job.

cburschka’s picture

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

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

babbage’s picture

StatusFileSize
new5.35 KB
Passed on all environments.
[ View ]

Re #95.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

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!

Cybergarou’s picture

Status:Fixed» Needs work

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.

tanoshimi’s picture

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

catch’s picture

Status:Needs work» Needs review
StatusFileSize
new737 bytes
PASSED: [[SimpleTest]]: [MySQL] 17,533 pass(es).
[ View ]

We can't introduce a check for PDO on every page request, so here's a patch for INSTALL.txt - at least that way if people go to the base URL and get a wsod, they might look in INSTALL.txt afterwards.

Sborsody’s picture

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.

catch’s picture

@Sborsody - that's probably best dealt with in the issue you opened since it's a slightly different problem to the fatal error which was removed here. I've subscribed over there.

Cybergarou’s picture

Status:Needs review» Needs work

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.

catch’s picture

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.

Leaving the current patch at cnr, we could may add something very small to bootstrap to check for class_exists('pdo') and redirect to index.php if it doesn't, but it's a real shame to have runtime code on every request for 1/10000 cases on bad hosting environments even if it's small.

Crell’s picture

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.

chx’s picture

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

chx’s picture

And both are insanely fast, yes.

willmoy’s picture

@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.

Cybergarou’s picture

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.

<?php
 
if($_SERVER['SCRIPT_NAME'] != "/install.php") {header( 'Location: /install.php' ) ;}
?>
David_Rothstein’s picture

@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:

<?php
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.]

cha0s’s picture

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)

Crell’s picture

Status:Needs work» Closed (duplicate)

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

David_Rothstein’s picture

Title:Add PDO to installation requirements» Installing Drupal by visiting index.php (rather than install.php) leads to a fatal error when PDO is not enabled
Status:Closed (duplicate)» Needs work

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.

catch’s picture

If we can do this in one place, along with the settings.php check, I withdraw any objection to checking on runtime, as long as it's only done once.

cha0s’s picture

StatusFileSize
new959 bytes
PASSED: [[SimpleTest]]: [MySQL] 17,901 pass(es).
[ View ]

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?

Cybergarou’s picture

Status:Needs work» Needs review

#115 works for me.

Status:Needs review» Needs work

The last submitted patch, 299308.patch, failed testing.

chx’s picture

BAHAHAHAAHA I love that approach.

cha0s’s picture

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 ;)

catch’s picture

Status:Needs work» Needs review

#115: 299308.patch queued for re-testing.

Crell’s picture

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.)

willmoy’s picture

Status:Needs review» Needs work

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.

kewlguy’s picture

+1 subscribing

Dig1’s picture

+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

webchick’s picture

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.

Sean Charles’s picture

Status:Closed (fixed)» Needs review
Issue tags:+installation, +Usability, +pdo, +beta blocker, +webchick's D7 alpha hit list
StatusFileSize
new4.12 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in 299308_1.patch.
[ View ]

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.

gordon’s picture

Status:Needs work» Needs review

subscribe

Status:Needs review» Needs work

The last submitted patch, 299308.patch, failed testing.

gordon’s picture

StatusFileSize
new3.96 KB
PASSED: [[SimpleTest]]: [MySQL] 19,938 pass(es).
[ View ]

Re-roll the patch

gordon’s picture

StatusFileSize
new3.96 KB
PASSED: [[SimpleTest]]: [MySQL] 19,944 pass(es).
[ View ]

Clean up the trailing white spaces.

rfay’s picture

Status:Needs work» Needs review
marcingy’s picture

StatusFileSize
new3.76 KB
PASSED: [[SimpleTest]]: [MySQL] 19,923 pass(es).
[ View ]

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

babbage’s picture

Status:Needs review» Reviewed & tested by the community

As per marcingy's instructions... :)

Berdir’s picture

Status:Reviewed & tested by the community» Needs review

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?

gordon’s picture

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

Berdir’s picture

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.

David_Rothstein’s picture

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.

cha0s’s picture

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.

gordon’s picture

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.

catch’s picture

Status:Needs review» Reviewed & tested by the community
-function _db_check_install_needed() {
-  global $databases;
-  if (empty($databases) && !drupal_installation_attempted()) {
-    include_once DRUPAL_ROOT . '/includes/install.inc';
-    install_goto('install.php');-function _db_check_install_needed() {
-  global $databases;
-  if (empty($databases) && !drupal_installation_attempted()) {
-    include_once DRUPAL_ROOT . '/includes/install.inc';
-    install_goto('install.php');
-  }
-}

-  }
-}

I don't see any dependency being added that's not already there either. If we want to go the code duplication route one day, the drupal prefix will make this easier to find anyway.

David_Rothstein’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new6.72 KB
PASSED: [[SimpleTest]]: [MySQL] 20,026 pass(es).
[ View ]

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.

cha0s’s picture

Status:Needs review» Reviewed & tested by the community

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

webchick’s picture

Status:Reviewed & tested by the community» Needs review

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.

gordon’s picture

Status:Needs review» Reviewed & tested by the community

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.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Excellent. Thanks, gordon!

Committed to HEAD! :D

Status:Fixed» Closed (fixed)
Issue tags:-installation, -Usability, -pdo, -beta blocker, -webchick's D7 alpha hit list

Automatically closed -- issue fixed for 2 weeks with no activity.

ianthomas_uk’s picture

Issue summary:View changes
Status:Needs review» Closed (fixed)

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