Problem/Motivation
If the user visits the site index for a Drupal site that has database credentials in settings.php but has not yet been installed completely, the user will receive a fatal error that does not in any way indicate that the user should visit install.php to complete the installation.
Proposed resolution
Detect an empty database during bootstrap, and redirect to install.php if installation is not already in progress.
Patch in #728702-17: Visiting index.php should redirect to install.php if settings.php already has database credentials but database is empty. is confirmed to resolve this issue for both D7 and D8.
Remaining tasks
None.
User interface changes
User will be redirected to install.php if there is a database configuration but the database is empty.
API changes
None.
Original report by @arpeggio
The setup are:
-database empty
-drupal updated from CVS
-set the C:\xampp\htdocs\sites\sites.php with $sites['localhost'] = 'kapitbisig.com';
-manually populated the the database credentials in C:\xampp\htdocs\sites\kapitbisig.com\settings.php
I got the following error message when I browsed http://localhost
Fatal error: Call to undefined function _system_rebuild_theme_data() in C:\xampp\htdocs\includes\theme.inc on line 586
Comment | File | Size | Author |
---|---|---|---|
#163 | 728702-163.patch | 22.16 KB | daffie |
Comments
Comment #1
arpeggio CreditAttribution: arpeggio commentedBTW, I would like to suggest if the settings.php is already populated with database credential and the database is empty, automatically redirect the user to http://localhost/install.php to avoid seeing this error. Thanks.
Comment #3
David_Rothstein CreditAttribution: David_Rothstein commentedI can definitely reproduce this. I had actually been meaning to file something about this for a while (some version of this bug actually dates back to Drupal 6, I think) but never bothered - so thanks :) It wouldn't shock me if there is already an existing issue for this, although I can't find one.
The current code in Drupal that redirects you to the installer when you visit the main site URL without having installed Drupal yet is this:
However, not only is this code not designed to catch the situation described in this issue, it also seems like we hit a fatal error before this function ever gets called...
Comment #4
arpeggio CreditAttribution: arpeggio commentedI see there's already a function handling this issue but it seems not get called. For a mean time, I suggest to have a note in settings.php something like:
So we can save the users from hair pulling :) I myself did it until I figured out the workaround is to use the http://example.com/install.php whew!
Comment #5
David_Rothstein CreditAttribution: David_Rothstein commented#906828: on fresh setup no redirect to installer script but PDOException table xy not found was duplicate.
Adding a slightly more descriptive title here.
Comment #6
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedWell, this saved me some hair-pulling, as I am currently setting up a settings.php file for a site, instead of having to input all the data during the installation. I will be sure to direct my browser directly to the install.php file.
Comment #7
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis is an important usability issue.
I suggest we check if the table exists on a cache miss in variable_initialize(). A cache miss there is sufficiently unlikely for this not to affect performance of any site.
Comment #9
Damien Tournoud CreditAttribution: Damien Tournoud commentedRight, we also need to check if we are not already in maintenance mode :)
Comment #10
Damien Tournoud CreditAttribution: Damien Tournoud commentedAnd the name of the table is 'variable' not 'variables'.
Comment #11
BarisW CreditAttribution: BarisW commentedDuplicate? See my issue of a few months ago: #890820: Redirect to install.php when database is empty
Comment #12
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis one is definitely older.
Comment #13
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedTested the patch and it worked like a charm.
Without patch: Visited the base url with database settings already defined in
settings.php
, and with a clean database, causes error.With patch: Successfully visited the base url with database settings already defined in
settings.php
, and with a clean database, lets me install like normal. After installation I can visit my new site through the base url like normal.This patch would save me from having to remember to explicitly specify
http://.../install.php
when I want to install my sites with a predefinedsettings.php
.Comment #14
David_Rothstein CreditAttribution: David_Rothstein commentedThe overall approach seems reasonable to me.
Instead of checking MAINTENANCE_MODE itself here (and specifically all values of it), I think it would be better to call drupal_installation_attempted() instead? That's what's done in similar code in _drupal_bootstrap_database() already, and it would be easier to understand also.
Comment #15
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #16
Tor Arne Thune CreditAttribution: Tor Arne Thune commented#10: 728702-redirect-to-install-empty-db.patch queued for re-testing.
Comment #17
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-roll including suggestion from #14.
Comment #18
pillarsdotnet CreditAttribution: pillarsdotnet commented#17: install_redirect_on_empty_database-728702-17.patch queued for re-testing.
Comment #19
pillarsdotnet CreditAttribution: pillarsdotnet commented#17: install_redirect_on_empty_database-728702-17.patch queued for re-testing.
Comment #20
catchWhy not put the lock_acquire() in a try/catch and do the db_table_exists() there?
Comment #21
David_Rothstein CreditAttribution: David_Rothstein commentedRerolled with that change.
Comment #22
pillarsdotnet CreditAttribution: pillarsdotnet commentedLooks good to me, and solves the problem for both d8 and d7.
Comment #24
pillarsdotnet CreditAttribution: pillarsdotnet commented#21: install_redirect_on_empty_database-728702-21.patch queued for re-testing.
Comment #25
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #26
xjm#21: install_redirect_on_empty_database-728702-21.patch queued for re-testing.
Comment #27
tstoecklerI didn't see this was RTBC already, so I tested it.
While I can't judge the code, it works beautifully!
RTBC++
Comment #28
klausidoes not apply anymore
Comment #29
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-rolled.
Comment #30
pillarsdotnet CreditAttribution: pillarsdotnet commentedBetter title.
Comment #31
tstoecklerBack to RTBC.
I checked that the patch is equivalent with #21.
Comment #32
tstoecklerHuh?, maybe this time...
Comment #33
catchWould it be a really bad idea to put this try/catch directly in index.php around the drupal_bootstrap? That way we don't need to move it around every time the first database query in bootstrap moves around.
Comment #34
pillarsdotnet CreditAttribution: pillarsdotnet commentedLike this?
Comment #36
pillarsdotnet CreditAttribution: pillarsdotnet commentedCorrected typo by adding the missing closing parenthesis.
Comment #37
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #38
BarisW CreditAttribution: BarisW commentedWorks great! Good work ;)
Comment #39
reglogge CreditAttribution: reglogge commentedI can confirm that the patch works. I emptied the database of an already installed Drupal and got redirected to install.php when hitting the site.
However, I found this behavior jarring. I got no notification that Drupal had trouble accessing the variables table in the database, in fact no notification that anything was amiss at all.
Wouldn't at least a message like "Drupal has problems accessing the database xyz, which is registered in settings.php. By selecting an installation profile, you can reinstall Drupal now. Yadda yadda yadda..." be in order?
Under what conditions could this behavior be triggered? Wouldn't anonymous users accessing the site with a missing variables table (for whatever reasons) be presented with the installer? Do we want that?
Comment #40
pillarsdotnet CreditAttribution: pillarsdotnet commented@reglogge
Sorry, but Drupal currently does not support passing messages through a redirect without database support. I suppose it would be possible using
$_GET
or$_SESSION
or via Javascript, but that would be a separate feature request, not part of this issue.Yes, and if you have a multisite install that lacks a
sites/default/settings.php
file, then anonymous users accessing the site via an unconfigured domain name will also be presented with the installer.Well, that's part of what this issue is trying to figure out. Feel free to offer your opinion.
Comment #41
tstoecklerWell a common scenario for production sites is that anonymous users do not have permissions to execute install.php, in which case they get a 403.
Comment #42
catch#1321656: DB problems installing 7.8: PDOException: SQLSTATE[42S02]: Base table or view not found: marked as duplicate.
Comment #43
xjmNote that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."Tagging as novice for the task of rerolling the Drupal 8.x patch.If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.Comment #44
xjmRighht, so, as reglogge points out, this may be the one patch in all of core that does not need a reroll. :)
Comment #52
reglogge CreditAttribution: reglogge commentedOh well. The patch doesn't apply anymore after all :-(
Rerolled and attached. This is a straight reroll from #36.
Comment #53
pillarsdotnet CreditAttribution: pillarsdotnet commentedShould be:
Comment #54
reglogge CreditAttribution: reglogge commentedof course.
Comment #55
xjmPruned an army of auto-generated testbot comments from yesterday.
I'm concerned that #52 didn't fail. Do we need test coverage for this?
Comment #56
pillarsdotnet CreditAttribution: pillarsdotnet commentedAsk rfay, but I'm not sure that this can be tested via Simpletest.
Comment #57
xjmMaybe just unit tests, if nothing else? We do have some bootstrap test coverage, though none for the installer that I could find.
Comment #58
pillarsdotnet CreditAttribution: pillarsdotnet commentedWell, let's see. You'd have to delete (at least) the
{variable}
table, then load a page and check to see if it redirects tocore/install.php
.So... something like:
installer.test
Comment #59
pillarsdotnet CreditAttribution: pillarsdotnet commentedBy the way, the patch in #54 is still wrong.
should be:
Comment #60
endyope CreditAttribution: endyope commented#9: 728702-redirect-to-install-empty-db.patch queued for re-testing.
Comment #61
pillarsdotnet CreditAttribution: pillarsdotnet commentedObservation in #59 is still valid.
Comment #62
tim.plunkettdrupalGet fails if the variable table is missing, so I switched it to system. It wasn't clear from the thread exactly WHY the variable table was chosen, so I hope this is still fine. Otherwise, I'm not sure how this would be tested.
Comment #63
xjmYeah, the installer is totally not very testable, which I did not know seven months ago.
Edit: #630446: Allow SimpleTest to test the non-interactive installer
Comment #64
tstoecklerThis looks really good, but since it has seen quite a few iterations and is a pretty low-level patch, I'm not going to mark RTBC myself.
Comment #65
xjmThanks @tim.plunkett and @tstoeckler.
This class needs a docblock.
I'm ishy on the whole rename-the-system-table thing, but it seems like the only solution for testing this.
It would also be good to do a round of manual testing in D8, and again in D7 if we backport.
Comment #66
ijf8090 CreditAttribution: ijf8090 commentedComment #67
tstoecklerNot removing the tag, but I tested this a couple times before. (We were checking for {variable} not {system} then, but that shouldn't matter.)
Comment #68
himerus CreditAttribution: himerus commented#62: drupal-728702-62.patch queued for re-testing.
Comment #70
sunI'd recommend to close this issue.
The redirect functionality in question does not work at all in D8 anymore, and after studying the code for quite some time as part of #1757536: Move settings.php to /settings directory, fold sites.php into settings.php, I'm relatively confident that this convenience redirect feature cannot be supported anymore. To install Drupal (8), you need to visit install.php.
Comment #71
pillarsdotnet CreditAttribution: pillarsdotnet commentedI guess it's too late for a D7-only patch?
Comment #71.0
pillarsdotnet CreditAttribution: pillarsdotnet commentedUpdated issue summary.
Comment #72
mgiffordThis is what is in Core now. Seems pretty close to what @tim.plunkett suggested in 2012.
Can we close this issue?
Comment #73
sunI'd be very hesitant to add even more of these checks...
In particular the proposed one here would be executed on all production sites in case the condition happens to be triggered for any kind of reason.
In essence, any kind of database error — including when the database server is temporarily down — will trigger the code path, greeting all visitors of your site with the Drupal installer.
Therefore, I'm actually strongly opposed to this proposal and would recommend to close this issue.
In fact, even the already existing check for (empty) database settings in the regular bootstrap is problematic from my perspective.
I think we're trying to do way too much "convenience" operations in the critical path of every single bootstrap.
Not to mention that it allows anyone to install another instance of Drupal on your server, in case the right conditions for a multi-site directory happen to be met (for whichever reason).
If you're installing Drupal, then you should know that you're install Drupal. Go to install.php.
Comment #74
milos.kroulik CreditAttribution: milos.kroulik commentedI find sun's comment from #73 very reasonable.
Comment #75
jhedstromBumping back to Drupal 7 since this is pretty well handled in 8 now. However, given #73, it might just be a 'wont-fix'?
Comment #76
pjcdawkins CreditAttribution: pjcdawkins commentedjhedstrom: is it well handled in D8? I can still reproduce this - after manually configuring everything in settings.php/settings.local.php, but before installing, I'm not redirected.
Comment #77
pjcdawkins CreditAttribution: pjcdawkins as a volunteer commentedI'm not convinced by #70 or #73. We can easily check whether something is a database error, as opposed to a core database table not existing. And regarding 'convenience', well, that's Drupal's raison d'être.
I see there is no longer a 'system' table.
I expect this could be a lot more efficient, and I'm not sure about how to write tests for this, but here's a PoC.
A couple of disclaimers:
install_verify_database_ready()
already checks for the existence of theurl_alias
table (it checks the last table defined in system_schema()), to see whether Drupal is already installed. I'm checking for the 'sessions' table, as that seems even more essential than URL aliases, and I didn't want to include thesystem.install
file.Comment #78
pjcdawkins CreditAttribution: pjcdawkins as a volunteer commentedA simpler patch - the try/catch was a red herring, detection is actually about whether table(s) exists, database exceptions are not relevant.
Comment #79
cweagansFrom #73:
We probably need to address this if this patch is going to move forward. I think it's convenient, but we need to make sure that we're not redirecting when the database is down or otherwise unreachable, and this behavior should be something that can be disabled in settings.php as well.
Comment #80
cweagansNote: patch in https://www.drupal.org/node/2581457#comment-10421415 also creates the drupal_is_installed() function, but slightly expanded and hardened using the contents of another existing function in core.
Comment #81
pjcdawkins CreditAttribution: pjcdawkins as a volunteer commentedRe. #79: that is no longer relevant, as I'm not triggering the redirect based on a PDOException. A database exception will just bubble up uncaught; no redirect will take place.
There is another possible problem, in that this introduces a database call during an earlier phase of bootstrap than normal. I'm not sure whether that's a genuine problem though.
Re. #80 OK, but the system_schema() function is only available if you include the system.install file, which I thought was not ideal (I mentioned that in #77).
Comment #82
cweagansre PDO exception - gotcha. Okay. That's probably fine then. I still think it ought to be something that can be disabled though. It's very rare that I'd actually want that behavior in anything other than a development environment. In fact, I'd go so far to say that it should be off by default and turned on in the example.settings.local.php, but that's just me.
Re extra db query - I don't think this is a problem. Part of the work that I'm going to end up doing tomorrow is to convert installation detection to a service, which means that it's swappable, so you can set up a NoOpInstallationCheck that always returns true to save the db query.
#81 - I think it's the most solid way to verify based on a DB table whether or not Drupal is installed. Plus, this file is already included at any point in the installer that you'd want to check if Drupal is already installed or not. It'll always be accurate, and we won't have to update it if tables are removed or renamed. I've still got some more work to do over in the other issue, but I'm going to have to tackle it tomorrow, as it's quite late right now.
Comment #83
pjcdawkins CreditAttribution: pjcdawkins at Centarro commented"It's very rare that I'd actually want that behavior in anything other than a development environment"
Yes I agree with that, if it can be enabled in settings.php. My use case is to provide a D8 demo/example, without users complaining that they see an exception when hitting
/
.Users who haven't got a settings.php yet won't encounter this problem, so disabled by default is good.
"Re extra db query - I don't think this is a problem."
Yes, well, it's certainly not a problem if you make it disabled by default.
Comment #84
pjcdawkins CreditAttribution: pjcdawkins as a volunteer commentedReroll of #78 FWIW - I haven't been following closely.
cweagans, have you made any progress on the other strategy?
Comment #85
pjcdawkins CreditAttribution: pjcdawkins as a volunteer commentedOops. Here's the actual patch.
Comment #86
mgiffordDoes this need the
<h3>Why this should be an RC target</h3>
info & RC phase evaluation table? Also there's the "rc target triage" tag.I'd like this in for the 8.0 release for sure.
Comment #87
cweagans@pjcdawkins, FYI, I uploaded a new patch on the other issue. I don't think we should hardcode a new database query into every page load without giving users a way to turn it off. The other patch essentially converts it to a service and uses it in the appropriate places.
Just note the other issue as a dependency of this one, and then this patch is basically only this hunk:
Comment #88
Rafael CansinosI am not sure if this help, but I had a similar problem, all the time redirect to install.php, with data base with some problems, but not empty, and I resolve it changing permission of settings.php to: chmod 444 settings.php. And it is important to have default.settings.php in default folder.
Comment #91
jonathanshawNeeds reroll for 8.1
Comment #92
pjcdawkins CreditAttribution: pjcdawkins at Platform.sh commentedRe-rolled
Comment #93
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedThe latest patch now apply.
Comment #94
Crell CreditAttribution: Crell at Platform.sh commentedI can confirm this behaves as expected when we run it on Platform.sh. The redirect happens without incident.
Comment #95
alexpottThe current patch will add an extra query during bootstrap for all sites. I'm not sure that that is acceptable. Is it possible to do the query and redirect in the exception handler? Therefore we'd only have the extra query if stuff was already going wrong.
Comment #96
hussainwebWe could also look at #2581457-5: Pre-configured settings.php with uninstalled Drupal causes the installer to refuse to move forward which makes it possible to override the database query.
Comment #97
Crell CreditAttribution: Crell at Platform.sh commentedHm. Fair point, Alex. Here's a completely new approach that catches the exception in DrupalKernel, instead.
We could quibble over what other "not installed" detections we want to include, but I think these are fine. I also tried trapping access denied, but that results in a redirect to the installer, which tells you the site is already installed. :-)
Patch is against 8.1.x, but should apply to 8.2.x as well, probably.
Comment #98
hussainwebWhen I tested this by clearing out the configured database, it did not throw an exception in
\Drupal\Core\DrupalKernel
at all. Instead, this is the exception thrown in Symfony's HttpKernel.This is returned as a regular response to
DrupalKernel
(with 404 code) which just prints it. The exception handler is not entered at all. I found this when I was trying the same approach as @Crell in #97.I'll try the patch anyway later but I thought I'd share my finding here.
Comment #99
hussainwebI just tried the patch in #97 and as expected, it didn't work. DrupalKernel's handleException is never called for this.
To test, I applied the patch and ran
drush sql-drop -y
for good measure. I then tried to load the website and got the same output as shown in #98 (i.e., without the patch).This is the reason I suggested looking into #2581457-5: Pre-configured settings.php with uninstalled Drupal causes the installer to refuse to move forward in #96.
Comment #100
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedOdd, I tried #97 (on 8.2.x) and it redirected fine for me.
Running the check in an exception handler is definitely an improvement - that was actually the direction the issue was headed previously (a mere 4 years ago...) but for some reason it was abandoned.
Clever. And hopefully safe? (i.e. no chance of a false-positive where this error code appears on a live site in the case of a "database down" situation?)
This is fragile because it relies on a particular query being the first one that Drupal runs. Why not do something like the earlier patches and just run a query to check this directly instead, e.g.
Database::getConnection()->schema()->tableExists('sessions')
?Earlier patches used https://api.drupal.org/api/drupal/core!includes!install.inc/function/ins... here, which has some stuff in it to make sure the redirect isn't treated as permanent. That might be important.
Comment #101
hussainwebDefinitely weird. Can you tell me how you tested this? I might be missing something.
I switched to 8.2.x and tried and it and it still didn't work. I made sure to run composer install and drush sql-drop both times.
Comment #102
hussainwebTo make sure, I cleaned everything and tested this again and it worked this time. I got the expected exception without the patch and then redirect with the patch. I am not sure what changed but it was the same sequence of commands I tried earlier. Anyway, I am going to leave it to needs work for comments in #100.
Comment #103
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI actually hadn't run composer install the last time I did a git update, so I tried it again with that (just to make sure) and I do get the same results - patch works on both 8.1.x and 8.2.x. Glad you are getting the same thing now :)
Comment #104
Crell CreditAttribution: Crell at Platform.sh commentedAttached patch addresses #100.3 and 4. Also, if we're going to send a 302 redirect it makes sense to do so for the other install-check as well.
PDO error codes are, unfortunately, grossly inconsistent. They're a combination of SQL-standard error codes and database-specific error codes. After some additional testing, here's a revised version. However, I'm not sure this is fully final yet.
Here's what my testing found, after installing normally and then breaking it in various ways:
1) If logged in and the database is missing, redirect to installer. OK.
2) If anonymous and database is missing, redirect to installer. OK.
3) If logged in and a table is missing, the first fail is for the session table, which happens at the bootstrap level, so this code catches it. OK.
4) If anonymous and a table is missing, the first fail is from PathProcessorFront, which gets caught by the exception listeners in place inside HttpKernel, and results in a 404 Response. NO error handling code in DrupalKernel runs at all.
I am unclear if this is sufficient; I suspect not, but I'm not certain of the best way to handle it. (But it's probably why the previous approach was more stable, since it made an active check rather than a passive check and thus didn't have this problem.)
Perhaps we need to duplicate this functionality in a kernel exception listener, which can depend on the database service and therefore do a session table check? That would add an SQL query to every exception path, although those are presumably more rare than kernel boot. Except on 404s/403s, which would then get an extra query. And on other database errors which may or may not then break as a result...
Comment #105
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThe fixes for #100.3 and 4 look good.
Oh, that completely explains what @hussainweb saw above then.
It's worth asking why that actually needs to be every exception rather than just database exceptions. PathProcessorFront::processInbound throws a NotFoundHttpException(), but you'd think it should fail earlier than that with a database exception (since it's trying to load config from the database but the config table doesn't even exist).
The main answer seems to be that DatabaseStorage::readMultiple catches exceptions silently. That doesn't seem right but trying to fix that here would probably lead down a rabbit hole, so I guess the conclusion is indeed that Drupal 8 will need to run the extra query on 404 pages (at the very least). It's not ideal but it doesn't seem that bad to me either, and still way better than the earlier patch which ran it on every request.
Comment #106
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedAdjusting issue credits (they were broken due to #2748147: All previous issue credits are being erased on some issues when a maintainer comments on it).
Comment #107
Crell CreditAttribution: Crell at Platform.sh commentedWell... nominally we can't assume that Drupal is running on SQL at all, can we? 99% of installs are, but we technically support running on all-non-SQL. At least in theory. :-) So if we do a blanket "on exception, if there's no sessions table then redirect to installer" then anyone on MongoDB would get redirected to the installer for any missing url, which seems undesireable... But to do more than that, we'd need to either do an affirmative check (query on every request) or get a more specific exception that we could check against, because yes the logic in processInbound() and readMultiple() right now is totally stupid.
But... eh, let's try this and see what happens. I'm going to be out of town for the next few days so if someone else wants to pick this up and run with it, feel free. (Or mark it RTBC and get it committed, that's good too.)
Comment #109
Crell CreditAttribution: Crell at Platform.sh commentedWell, that didn't work... I am guessing from the test log that the problem is we're redirecting to install way too often, which is what I was afraid of.
Comment #110
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI went ahead and played with this a bit and came up with the attached version:
tableExists('session')
=>tableExists('sessions')
which is an important one :)There seem to be at least eight different scenarios for how the redirect can happen (!), which include all possible combinations of:
All eight seem to work with this new patch based on manual testing (on 8.1.x), and I also tested that exceptions on an already-installed site (e.g. 404 pages, database connection failures, etc.) don't trigger the redirect.
If we can write automated tests for at least some of those scenarios that would be nice... In the meantime let's see if it passes existing tests.
Regarding sites that don't store their sessions in an SQL database (or that don't use SQL at all) it seems to me like that's actually covered? I wrote this in the patch:
If sessions aren't in an SQL database, the sessions table may be empty/unused, but based on how it gets created during install it seems to me like tableExists('sessions') always should return TRUE on an installed site, unless someone went in and manually deleted it by hand. But if I'm wrong we can add back the comment about that limitation.
Comment #112
alexpott@David_Rothstein #110 looks really nice and complete... one thought:
I think rather than doing this here we could delegate at this point to the database driver / connection class. The default implementation would be as we see here - so mysql, psql, sqlite all do this (i.e. check sessions table and this particular exception code) but something like Mongo or whatever else can do it's own thing.
Comment #113
Crell CreditAttribution: Crell at Platform.sh commentedNice consolidation, David! Just a few minor points:
A side effect of this is that even on a non-SQL-based site, we will still need to initialize an empty SQL connection on every exception. That likely makes this subscriber another class that a non-SQL-based site needs to rip out entirely. That should be documented somehow, at least in the class's docblock.
Why?
It would be nice if we could somehow see what happens on an entirely non-SQL installation, since we do technically enable that in D8. That said, I'm unclear what automated tests for this would look like that aren't too micro-level to really test that the integrated behavior is correct.
Comment #114
Crell CreditAttribution: Crell at Platform.sh commentedSilly bot...
Comment #115
daffie CreditAttribution: daffie commentedFixed the point from alexpott from comment #112 and the second point from crell from comment #113.
Comment #116
daffie CreditAttribution: daffie commentedSorry, wrong interdiff.
Comment #118
daffie CreditAttribution: daffie commentedOops
Comment #119
Crell CreditAttribution: Crell at Platform.sh commentedNo idea why it hiccuped on Postgres before, but it seems to be working now and the code looks OK. Thanks, daffie!
Comment #120
tstoecklerIn
Connection::databaseEmpty()
:This should not mention "redirecting" as that is not within the function's scope.
AFAIK this can never be reached, so we can just drop it I think.
In
CheckInstalledTrait::shouldRedirectToInstaller()
:I guess this is where the
return FALSE
should go (maybe with a different comment, though), as this function - per its docs - should return a boolean.Comment #121
Crell CreditAttribution: Crell at Platform.sh commentedHm, you're right. In that case, however, let's move the databaseEmpty() check out of the connection object (since it's doing a very Drupal-specific check, and we want to keep the DB system not Drupal-dependent) and put it into the trait, where the comment is valid.
Comment #122
daffie CreditAttribution: daffie commentedNitpick.
Should this then become:
@return bool|null
Comment #123
tstoecklerI think #121 looks much better, thanks!
I don't quite get #122, in #121 the function does always return a boolean AFAICT.
Comment #124
daffie CreditAttribution: daffie commentedMy patch from comment #118 was RTBC for user Crell.
The changes made in the patch from comment #121 are for me RTBC.
I have one minor documentation change that can be done on commit and that is to the documentation of the return value CheckInstalledTrait::databaseEmpty():
@return bool|null
. The return value can be null.Comment #125
tstoecklerCan you explain in which case NULL will be returned? I don't see it...
Comment #126
daffie CreditAttribution: daffie commented@tstoeckler: If an exception is thrown wail fetching the session table existence and you get an error that is not 1049, the proces will run to the end of the method and return null. Please correct me if I am wrong.
Comment #127
tstoecklerIf an exception is thrown while fetching the sessions table, then we reach the
catch()
clause. That contains thereturn
statement as the only thing that will run - and it will always run. No if statement or anything similar. If the error code is not 1049 - for example - the boolean in the return statement will yield FALSE so that will be returned, not NULL.Comment #128
daffie CreditAttribution: daffie commented@tstoeckler: I was wrong and you are right. Thank you for the explanation.
Comment #129
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented#121 looks similar to #110 but is missing code that was there to deal with some of the scenarios; so for example, if the database doesn't exist, #110 (and also Crell's earlier patch in #104) works, but #121 doesn't.
It may still be possible to simplify #110... but I assume the issue is that if the database doesn't even exist, Drupal is incapable of establishing a connection and therefore no $connection object gets created? (I haven't actually debugged it.) That is why the code in #110 dealt with that scenario by falling back on examining the passed-in exception object instead.
Also:
Comment #130
Crell CreditAttribution: Crell at Platform.sh commentedThe issue was the check of the exception itself to see if it's a missing-DB only happened inside databaseEmpty(), which is only called if there is a $connection, which there isn't if the database is missing. Go team! :-)
So let's break that up and do the missing-DB check first, at which point we don't need to bother with the separate method anyway. This works in my manual testing.
David: A non-SQL backend should not be using a Database\Connection object. In Drupal 7 such hackery was necessary, along with some black magic and sacrificing of goats. In Drupal 8, use of Database\Connection for anything that isn't an SQL driver is completely wrong and entirely unsupported. (Well, that was true in Drupal 7, too, but there wasn't an alternative.) Swapping out services at a higher level is the correct way to handle that in Drupal 8.
Which means this code SHOULD work for all cases where Drupal has an SQL backend, and the sessions table exists (even if it's not being used). If Drupal is operating in a completely non-SQL environment, I think it will treat all exceptions as install issues, as it will either say there's no DB credentials at all or they are invalid/missing DB. I... am not sure how to address that.
Comment #131
dawehnerIs this check really super reliable? Isn't the 1049 coming from mysql itself and is not really a thing in pgsql/sqlite?
Comment #132
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThat does seem to work, but it makes the assumption that this function will only ever get a NotFoundHttpException passed to it when the database exists. The previous code did not assume this so was a little more robust. (In practice, though, it appears to always be the case.)
That code comment doesn't really make sense, since all this code does is redirect to the installer. (Hopefully there is no scenario in which the installer lets you overwrite an already-installed database... but either way, the above code does nothing to minimize the chance of it happening.)
Could replace with something like "... to minimize the chance of redirecting to the installer on an already-installed site."
It could be, although this patch would still be pretty useful even if it only worked on MySQL. I guess the big question would be whether 1049 means a different kind of error on some other database system (since then you could have a false redirect there). A quick web search for "1049" for PostgreSQL and SQLite did not turn up anything one way or the other.
Comment #133
Crell CreditAttribution: Crell at Platform.sh commentedYou know what, this is silly. We *already have* a DatabaseNotFoundException, why aren't we using it?
Let's use it. I also added an access denied exception while I was at it, although I realized we shouldn't use that in the redirect check since the installer won't run in that case anyway. I left the new exception in because, really, we ought to have done that in the first place 8 years ago anyway.
#132.1: If this method is called for a not-found exception, it will still run through the same checks; if it's a not-found exception that's not due to a missing or empty database, all the checks will fail and we'll return false (most likely on the tableExists check inside the try block), and the rest of the exception handling process will happen normally. This case is only to handle the, frankly, bug in PathProcessorFront, which we don't want to dig into here as it's a separate rabbit hole.
Comment #135
Crell CreditAttribution: Crell at Platform.sh commentedIt amuses me that would be the issue. It amuses me even more that there was a spelling error that's been there since who knows when. :-)
Comment #136
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedNeeds some manual testing, but at first glance those changes make a whole lot of sense.
I think the order of the above two checks should be reversed, no?
I wonder if it's possible/worthwhile to write tests specifically for this function (like pseudo-unit tests).... There are so many combinations of scenarios that can occur here now, and although actual functional tests would be complicated (and probably impossible for some of the scenarios) testing this function directly would be easier.
Comment #137
daffie CreditAttribution: daffie commentedBoth points from David from comment #136 are addressed. Changed the order in CheckInstalledTrait a bit more.
Comment #139
daffie CreditAttribution: daffie commentedI created the new method Drupal\Core\Installer\CheckInstalledTrait::isCli() so that it can be mocked in PHPUnit testing.
Comment #140
daffie CreditAttribution: daffie commentedI made a patch error in comment #137.
Comment #142
daffie CreditAttribution: daffie commentedJust the bugfix for UncaughtExceptionTest.
Comment #143
daffie CreditAttribution: daffie commentedComment #144
neclimdulClearly your failures are coming from this comment saying its thrown when the database is not found but its thrown when access is denied.
Wish I could be of more help but those are weird.
Comment #145
Crell CreditAttribution: Crell at Platform.sh commentedI talked with amateescu in IRC, and he pointed out that the Postgres PDO error codes are horribly unreliable. See: #2371709: Move the on-demand-table creation into the database API, where he and chx addressed the problem with a utility method to parse out the SQLState code from the error message, Because that's the best we can do. Because BAH!
So here's an updated version that gets the error code that way, using the code from that issue, and corrects the code being used based on amateescu's tests.
I have no idea idea what's up with #142, honestly. That's just weird.
Oh yeah, and I also fixed the super-critical issue that neclimdul pointed out in #144. :-P
Let's see how the bot likes this one.
Comment #146
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedInvestigated this for a while and found out that the problem is that for Postgres the error code (7) and the sql state code (08006) are the same for both exceptions, it's just the exception message that differs:
So the only reasonable way to proceed is... wait for it... more string parsing! :)
Comment #147
daffie CreditAttribution: daffie commentedI performed the following manual checks:
The last two responses do not apply for SQLite, because that database does not have that kind of access checking.
If there needs to be checked more for "Needs manual testing" then let me know. I removed the manual testing tag because my main addition to the current patch is the testing class. If someone does not agree with this then please put back the tag.
Comment #148
jonathanshawShouldn't this be against 8.2.x now?
Comment #149
Crell CreditAttribution: Crell at Platform.sh commentedThe fails against 5.6 are HEAD fails and do not block this issue, per Alexpott: #2762549: Drupal\field\Tests\Update\FieldUpdateTest, Drupal\views\Tests\Update\EntityViewsDataUpdateTest and Drupal\comment\Tests\CommentFieldsTest fail on 8.1.x.
So the patch is now working. Someone please RTBC so we can get it committed! :-)
Comment #150
daffie CreditAttribution: daffie commented@Crell: This issue is for me RTBC. With the exception of the added test. Which I wrote. If you think my test is RTBC, then for me is the whole issue RTBC.
Comment #151
alexpottThe name and description of the trait and it's only useful method don't really tie. It doesn't provide methods to check if Drupal is installed. It provides a method to check if you should redirect to the installer. So about InstallerRedirectTrait?
Comment #152
Crell CreditAttribution: Crell at Platform.sh commentedRenamed per #151. Also, while I was at it I noticed that the test for the trait was using string literal classes. We should be using ::class constants now for PHP 5.5, which allows the class name to be parsed by the engine or by an IDE, which in turn makes this sort of refactoring easier. :-) (I nearly didn't catch that the name of the trait was in a string in that file and therefore wasn't picked up by PHPStorm's Refactor command.)
No functional change, just naming things.
Comment #153
daffie CreditAttribution: daffie commented@Crell: You made some changes to the test that I wrote. Thank you, because I learned something new.
I think the patch looks great now and for me it is now RTBC. Only I have written some of the patch so I cannot set the status to RTBC.
Comment #154
daffie CreditAttribution: daffie commentedChanged my mind. @crell reviewed my the part that I wrote (the InstallerRedirectTraitTest) with the changes that he made to it, in comment #152. The rest of the patch is RTBC for me. I did the manual testing in comment #147.
If somebody does not agree with this, then please change the status back to "Needs review".
Comment #156
daffie CreditAttribution: daffie commentedBack to RTBC.
Comment #157
daffie CreditAttribution: daffie commentedBack to RTBC.
Comment #158
alexpottNeeds to be mentioned in a CR for alternate DB engines since they might not have an SQLState and might need to do something different.
Alternate DB Drivers will need to implement something like this - no? - This likes more for the change record to mention.
Comment #159
daffie CreditAttribution: daffie commentedAdded a change record.
Comment #161
alexpottGiven the changes this patch makes and the need for CR I think we can only release this in a minor version so bumping the version. Also the patch needs a reroll.
Comment #162
Crell CreditAttribution: Crell at Platform.sh commentedAlex: A CR was added 2 weeks ago, and the patch RTBCed initially 2 months ago and sat for a month without comment. Without this patch, Drupal 8.2 will continue to be uninstallable on Platform.sh. (We currently apply an older version of the patch from higher in the thread in our composer.json file.) Another 7 months of needing a now non-small patch to install Drupal on a read-only file system does not strike me as a good situation for anyone involved, but I don't know what exactly we could have done earlier or can do now.
Edit: Correction, it's installable, but it's poor UX since you have to manually go to /core/install.php. Still frustrating given how long this patch has been sitting idle. :-(
Comment #163
daffie CreditAttribution: daffie commentedRerolled the patch.
Comment #164
cilefen CreditAttribution: cilefen commentedOnly after looking twice did I realize the CR actually does refer to this issue.
Comment #165
Crell CreditAttribution: Crell at Platform.sh commentedWe just successfully tested and deployed #163 on Platform.sh with 8.1.10. Thanks, daffie!
Edit: Spoke too soon; it's #152 that still applies and works on 8.1.10. I'm just all over jumping the gun today. :-(
Comment #166
alexpottCommitted 29ce04e and pushed to 8.3.x. Thanks!
@Crell back Drupal has behaved this way for 7 years. Yes it is great that this will be fixed in the future but it seems odd that Platform.sh is really a behaviour that has never been there.
Please don't change files perms in patches... fixed back to 644 on commit.
Fixed the following coding standards violations on commit.
Comment #168
Mile23Interestingly, Drupal\Tests\Core\Installer\InstallerRedirectTraitTest causes a fatal when run from PHPUnit, but not run-tests.sh, for 8.3.x.
Basically the test is testing code that has a hard dependency on a bootstrapped kernel, so it should be a KTB test instead of a unit test.
Follow-up issue: #2805757: InstallerRedirectTraitTest causes fatal when run from PHPUnit
Comment #169
alexpottIt is possible that this change has introduced a new random fail. https://www.drupal.org/pift-ci-job/477829
Comment #170
alexpottCreated #2806697: Random fail for AlreadyInstalledException
Comment #172
heddnIf we plan to backport this to D7, can this also get backported to 8.2?
Comment #173
cilefen CreditAttribution: cilefen commentedIt applies cleanly.
Comment #174
heddnMoving to RTBC to see if we can get this into 8.2.x. Is that the right status?
Comment #176
heddnComment #177
heddnTests pass on 8.2. Moving to RTBC.
Comment #178
alexpott@heddn in #161 I wrote that this change was making additions only suitable for a new minor version. These changes are:
New service
New method on a class that is subclassed for alternative DB drivers.
New exception type.
Comment #180
esteinborn CreditAttribution: esteinborn commentedFYI, Neither patch #152 nor #163 applies cleanly to 8.7.1.
Comment #181
cilefen CreditAttribution: cilefen commentedIt should not. We merged this years ago.
Comment #182
esteinborn CreditAttribution: esteinborn commentedI guess I'm confused as to what this fixed then.
On a fresh install with a settings.php file with filled in DB credentials that point to an empty database, I get an error on index.php, no redirect to /core/install.php. then when I visit /core/install.php it tells me that Drupal is already installed (It is not, it's a completely empty database).
Comment #183
esteinborn CreditAttribution: esteinborn commentedPlease ignore my last post. I had and issue with MySQL 8.
Comment #184
joseph.olstadbackport issue