dev/drupal/#137 updates hook requirements adds sanity check, remove install checks

[stesi561]

hook_requirements didn't account for it being called post install. This PR adds a very quick sanity check - is civicrm initialised and is civicrm.settings.php writable.

The check on is civicrm initialised to match behaviour on D7.
It's possible we should add some behaviour here to catch the case where the installer hasn't run? However as the installer isn't being used on D8 currently - and it installs on enabling this is probably surplus to requirements for now.

Rationale for adding the check on civicrm.settings.php (and nothing else) is due to the previous behaviour which warned if it was not writable. Potentially this could be an error? But have left as a warning for now.

[demeritcowboy]

I can take a look at this tomorrow.

Regarding the installer there is cv core:install and cv core:check-req which work currently, and I expect eventually enabling the module will take you to a UI page that lets you pick more parameters, like has been done already for some cms's. So I'll check what happens.

[stesi561]

I wasn't aware of cv core:check-req however a quick check of the code suggests it is running the same checks that happen before this patch is applied.

While these look promising I think we run into the same hurdles. I think the best solution would be that the core checkRequirements is modified to take an option of checking an existing install and modifies some of the checks. The goal with this patch is not to require a writeable civicrm.settings.php to avoid seeing errors when hooks_requirements is run. I think

[demeritcowboy]

Thanks for the PR - overall it seems to address the problem just some issues below.

• General standards
  • [r-explain] PASS
  • [r-user] PASS
  • [r-run] PASS
    • The reverse check as a warning seems ok and is probably good security practice. To me it doesn't rise to the level of error because civi doesn't require the file to be locked down in order to function, and it might complicate uninstalling civi albeit easily dealt with (and who uninstalls civi?!).
    • For the private files issue, it solves the issue because it's not running the check anymore, so agree with the todo comment that it might be good to add other modified checks.
  • [r-doc] Possible confusion
    • The warning gives a link to the install docs, which in fact says the opposite that it needs to be writable and in the post-install permission steps doesn't say anything about this.
• Developer standards
  • [r-tech] Comment
    • I wonder if people are using the UI status page to confirm the other checks which would no longer be run and will think their system is ok. Not directly after install, but sometime well after install when something changes and they're having some problem. It wouldn't prevent them from finding the cause, just might take them longer.
    • Regarding cv core:check-req, I think I agree that separately it could also be updated to be consistent with this when run while civi is already installed.
  • [r-code] Issue
    • I don't think t() should be used here and maybe @mlutfy can confirm. This is one of those sysadmin situations where the person might need to do a web search or grep so having the english text seems better. See also lower down in the file where t() is not used.
    • Typo in line 45 - missing an "i" in initialized.
    • Line 48 - spelling of Civicrm => CiviCRM
    • If you run phpcs on it several items come up:

      FOUND 9 ERRORS AFFECTING 9 LINES
      42 | ERROR | [x] Expected 1 space after "="; 2 found
      46 | ERROR | [x] Array indentation error, expected 8 spaces but found 6
      47 | ERROR | [x] Array indentation error, expected 8 spaces but found 6
      48 | ERROR | [x] Array indentation error, expected 8 spaces but found 6
      56 | ERROR | [x] Expected 1 space after closing parenthesis; found 0
      58 | ERROR | [x] Array indentation error, expected 8 spaces but found 6
      59 | ERROR | [x] Array indentation error, expected 8 spaces but found 6
      60 | ERROR | [x] Array indentation error, expected 8 spaces but found 6
      70 | ERROR | [x] Functions must not contain multiple empty lines in a row; found 2 empty lines
      

  • [r-maint] __ __
  • [r-test] PASS

[demeritcowboy]

Another thought - I wonder if we're doing this backwards and should instead be putting something in here that does something slightly different if civi is already installed:

https://github.com/civicrm/civicrm-core/blob/28c2015d6a7b4b00517d74b9df7361b1cf7bc925/setup/plugins/installFiles/InstallSettingsFile.civi-setup.php#L31

https://github.com/civicrm/civicrm-core/blob/3ed73abbc9d62870aa2422336e8fc2957857b79d/setup/plugins/installFiles/CreateTemplateCompilePath.civi-setup.php#L24

That way the other checks that still make sense when installed would still run as-is. And if this were run from non-drupal you'd want that too.

[demeritcowboy]

Thanks for moving this along. My main concern is that this prevents all the other good checks from running when you visit the drupal status report. I have an alternate which I'll put up in a minute.

[stesi561]

Sure - I think the best solution would be that core is aware of install status and issues warnings accordingly.