Installer does not verify readability of settings.php

I started with a clean git of 8.x-dev. when I started the installation I observed the error. Verified that the bug still exists.

Found this issue, downloaded the patch from http://drupal.org/files/settings-readable-check-1805324-1.patch
Made a new branch , applied the patch in the new branch.
Deleted my setting.php in order to restart the install.
Went back to initial installation of this Drupal install,

The error did not occur. Proceded directly to the Database setup page rather than having the error.

This patch fixes the bug.

Next step is to get a code review.

Patch seems sane and fixes the problem.

It would be great to get a couple of more reviews on this, from sun or chx for example.

So what happens to $readable? It starts as FALSE and in current HEAD only gets flipped to TRUE if the dir and settings file both exist already. Otherwise we copy the default to settings.php and... forget to set $readable. There's some dance happening after the copying which is amply documented but anyways indeed you will get a readable error because we do not set $readable and when you reload then it'll be set because again, that's the only way it is -- when the file already exists. The patch is correct.

Didn't test it out, but reading the patch and surrounding code it makes sense to me also.

I was confused because I was quite sure based on previous experience that this bug didn't actually exist (for example, in Drupal 7), so I checked a bit and it turns out it was introduced relatively recently (and for Drupal 8 only) via #1542620: Fatal errors when file permissions for settings.php are messed up (can't read file)

Yep, this is essentially the same fix as in #1798732: Convert install_task, install_time and install_current_batch to use the state system

That said, I studied what the called code is doing (which is anything but trivial) and concluded that the returned value can be assigned to both $readable and $writeable, as in:

$readable = $writable = drupal_verify_install_file($settings_file, FILE_READABLE|FILE_WRITABLE);

However, we can also check them separately — doesn't make much of a difference.

Right, that works technically too. But I think it's against our coding standards to do double assignments like that. Don't know if it's documented but I know patches of me were set to needs work because of such things.

Yeah, very sad panda. Commenting on this would only circle back into #1791872: [Policy, no patch] Add special tag to identify issues ready for a high level only review... so I refrain from that and that's why I'm fine with the duplicate invocation.

Someone will ask whether we cannot do a simple $readable = $writable; assignment on a second line, but that would be even more confusing than the double variable assignment. At least in my book.

Since I was reading the comments to catch up on the progress here, I updated the issue summary.

@webchick, I think your concerns from #4 have been address and this is ready.

Oh, nice! It turns out this issue has been driving me nuts while I've been testing Spark. :) Nice to see a fix! And thanks for the extra reviews.

Committed and pushed to 8.x. Thanks!