dev/core#5484 SettingsManager - reinstate intermediary boot phase...

[ufundo]

... to allow reading extension container settings from the database.

Overview

This is a more thoughtful alternative to https://github.com/civicrm/civicrm-core/pull/31169/files in order to fix https://lab.civicrm.org/dev/core/-/issues/5484

Before

SettingsManager has 2 states:

1. preBoot = only core boot settings (extension hooks have not been called), only values from env var or civicrm.settings.php (no database values)
2. bootComplete = fully online

A database value for extensionDir isn't respected when booting the extension system.

After

SettingsManager has 3 states:

1. preBoot and preDB = preBoot as above
2. preBoot but DB online = extension hooks have not been called, but values are read from the database
3. bootComplete = fully online

Database values for extensionDir , extensionUrl and ext_max_depth can be respected again when booting the extension system.

Comments

I preferred the cleaner separation before. I wonder if in the long run we could deprecate storing the extension system config values in the database? To me they feel like they should be in civicrm.settings.php / environment variables. They are very closely coupled with the arrangement of files on the system.

[civibot]

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...

• If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
• A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
• A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
• If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
• Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
• A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
• To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
• For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers...

• ➡️ PR Demo site (can be rebuilt with the command test this please).
• 📖 Review standards
• 🗒️ Review template (brief or verbose)

➡️ Online demo of this PR 🔗

[demeritcowboy]

Thanks will give it a try.

[demeritcowboy]

This works for me.

On the deprecation, I'd agree if you're changing these particular settings, you're obviously managing the filesystem already. But I dunno if I'm the right person to ask since for any question about where config should live I'd lean towards files.

[totten]

I preferred the cleaner separation before. I wonder if in the long run we could deprecate storing the extension system config values in the database? To me they feel like they should be in civicrm.settings.php / environment variables. They are very closely coupled with the arrangement of files on the system.

• Yeah, exactly, storing extensionsDir (or any other boot-critical setting) in the database makes bootstrap more complicated -- and there isn't much benefit to it.
• IMHO, just about everything in "Admin > System > Directories" and "Admin > System > Resource URLs" should be in a config-file -- not in database, not in UI. (Well, the UI could have hints to point you to the right files... to improve discovery/learning... but that's by-the-by.)
• Agree that config-file and env-var are both better than DB-settings. Of course, these approaches are "AND" not "XOR". (They differ in suitability to specific deployment-styles and specific config-items.)
• The real obstacle to changing this is the migration of existing deployments and their settings. The basic/natural thing is to read the old setting and put an equivalent snippet in civicrm.settings.php. However, that kind of migration requires careful attention (because the file-format is so flexible... and file-permissions are variable... and any mis-step leaves you with a broken system...)

--

Anyway, to the main point -- +1 to the general concept of PR. It makes sense that bootstrap needs this level of nuance.

[ufundo]

The real obstacle to changing this is the migration of existing deployments and their settings.

How about:

• make a Gitlab issue to check for consensus that these should be deprecated
• add a status message warning if db value differs from mandatory value/default, telling you what line you should add to civicrm.settings.php
• leave it there for 6 months (or 12 months if we're being uber cautious)
• remove support for db setting (and revert this PR). include an upgrade notice in the release notes
  ?

The basic/natural thing is to read the old setting and put an equivalent snippet in civicrm.settings.php

Is this actual possible? If they have a custom e.g. extension directory when they switch in the new code version it may well crash, and then they won't be able to run the upgrader? I suppose you could have the upgrader in a few versions before you remove support. But that has the same issue as above that people might be upgrading from below that version in a big jump, and skip the intermediary warning/migration step?