-
-
Notifications
You must be signed in to change notification settings - Fork 817
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add *.boot.setting.php meta for CIVICRM_UF_BASEURL and CIVICRM_DOMAIN_ID #31155
base: master
Are you sure you want to change the base?
Conversation
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
16fa691
to
c382348
Compare
@ufundo - I tried this out as follows:
I wasn't sure where /var/www/html/setting-stub should exist or not so I removed it but got the same message. I also tried passing CIVICRM_DSN instead of, and as well as CIVICRM_DB_* variables but but got the same message. Let me know if I am missing something or you want me to test again. |
Sorry @michaelmcandrew that doesnt exist as an env var (yet) For now I got it working by adding
at the top of civicrm.standalone.php |
I can confirm that adding that line works for me :) I was able to log in successfully. |
c382348
to
4d51467
Compare
retest this please |
I've marked this merge-on-pass based on user testing, and hit "retry" on the failing unit tests. @ufundo can you take a look at any test failures when the runs complete. |
Oh yes there's also an issue with settings style conformance:
If I "add" them using current versions, then it wants the metadata to have prefix. But the pre-existing constants don't obviously conform. I think I could backdate the "add" to opt out of the conformance. Or I could make them fit the existing pattern, though it will be more deviation from the current names. |
@ufundo whatever's cleanest. Opting out seems reasonable. |
4d51467
to
a58482f
Compare
…e loading from env/default
a58482f
to
d90b093
Compare
@colemanw something strange was going on with CIVICRM_UF and cv, so I've backed out of that one for now, and just stuck with CIVICRM_DOMAIN_ID and CIVICRM_UF_BASEURL. Hope still ok to MOP with the reduced scope. Step by step. |
Overview
EDIT: Jenkins / cv bootstrap didn't like the addition of CIVICRM_UF as a defined env var, for reasons I can't quite pin down - so I've rolled that back for now
Now just adds CIVICRM_UF_BASEURL and CIVICRM_DOMAIN_ID as globals, which means:
a) they can be provided using environment variables,
b) CIVICRM_DOMAIN_ID will default to 1 if not set (so the
define('CIVICRM_DOMAIN_ID', 1)
is not required in civicrm.settings.php)Before
After
Technical details
All the above applies only to Standalone, as that is where we have the SettingsManager::bootSettings function wired into the bootstrap, in order to source env vars for these kind of things early enough.
I've put in an explicit opt out from the naming convention in settings style test, instead of a backdated value for the version
add
ed....
Previous bolder scope:
Following on from #30533 - this brings a few more constants into the
*.boot.setting.php
system, in order that they can be loaded from environment variables or defaults.It's based upon #31150 - between the two I was able to run Standalone without civicrm.settings.php - just providing a three env vars.
Before
civicrm.settings.php
provides many critical constantsAfter
CIVICRM_DSN
,CIVICRM_UF_BASEURL
andCIVICRM_UF
as environment variablescivicrm.settings.php
Comments
It's running Smarty2, but it's running....
@michaelmcandrew could you have a go testing this branch with the Docker container?