invalid conf file warnings when skip_permissions_hardening is on

Reading through the code a bit more, I can now see a reason for the existing logic:

https://api.drupal.org/api/drupal/core%21includes%21install.inc/function...

...actually tries to fix the permissions of the file according to the masks it is passed, so the existing code doesn't run the check with that function as doing so may change the existing permissions.

In which case, I'd argue we should skip the checks without issuing the warnings.

Patch which skips the checks on config files - which is not actually a change to the behaviour, we just no longer flag up permissions warnings without actually checking those permissions.

It seems to me that some of the comments in the surrounding code are also a little bit misleading:

  // Test configuration files and directory for writability.                        
  if ($phase == 'runtime') { 

...this doesn't mention that the code is about to try and fix any incorrect permissions.

      // In normal operation, writable files or directories are an error.           
      $file_protection_severity = REQUIREMENT_ERROR;                                
      if (!drupal_verify_install_file($site_path, FILE_NOT_WRITABLE, 'dir')) {

...because drupal_verify_install_file actually tries to change the permissions if they don't initially match the mask, we only actually consider it an error if that attempt to fix the permissions fails.

I wonder if we should add a word or two of extra explanation about what the "hardening" actually is here - i.e. attempts will be made to fix incorrect permissions?

Oops; and now with a tweak to the test because the warning produced by system_requirements is slightly different.

So let's make the warning that's shown when skip_permissions_hardening is enabled more explicit about the situation.

     if (Settings::get('skip_permissions_hardening')) {
-      $conf_errors[] = t('Protection disabled');
-      // If permissions hardening is disabled, then only show a warning for a
-      // writable file, as a reminder, rather than an error.
+      $conf_errors[] = t('Protection disabled: the skip_permissions_hardening setting is not recommended in production environments.');
+      // If permissions hardening is disabled, then only show a warning as a
+      // reminder, rather than an error. The config files are not actually
+      // checked as doing so may change their permissions.

The problem is that we don't know whether those config files are "protected from modifications" (i.e. are writable) or not; calling the function to verify that will change the permissions if they are writable.

I think we should avoid giving incorrect information (i.e. saying "settings.php is writable" when that may not be the case) as that just leads to confusion and frustration.

We could try to go the extra mile and actually check the permissions without trying to fix them, but that would mean using something other than drupal_verify_install_file.

I'm not sure it's worth the extra complexity of checking the permissions in a different way; the idea is simply to remind developers that they have disabled the "permissions hardening" locally, and that they'll want to ensure that setting is not carried over to production.

My vote is avoid saying anything about the actual state of specific config files because the code (as it stands) doesn't know what their permissions are.

I think the patch in #16 looks good; I've verified that a writable settings.php file goes from producing an error to a warning when skip_permissions_hardening is enabled.

It's an enhancement to be able to call drupal_verify_install_file() without having it actually change permissions on files when appropriate.

I'll take a look at drafting a change record for this.

Hmm, testbot snafu?

Draft change record: https://www.drupal.org/node/2961774

NR for the change record.

testbot snafu?