Skip to content
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

Use full setting path rather than relative #668

Merged
merged 1 commit into from
Jul 27, 2023

Conversation

eileenmcnaughton
Copy link
Contributor

This makes the setting of CIVICRM_SETTINGS_PATH consistent with other places and addresses a build bug

This makes the setting of CIVICRM_SETTINGS_PATH consistent with other places
and addresses a build bug
@civibot
Copy link

civibot bot commented Jul 27, 2023

(Standard links)

@civibot civibot bot added the 7.x-master label Jul 27, 2023
@totten
Copy link
Member

totten commented Jul 27, 2023

I'm not really sure if absolute or relative is better, but consistency sounds good. With this patch, the following boot protocols would be in agreement:

  • CMS-first boot for Drupal (PR changes it to absolute)
  • CMS-first boot for WP (already uses absolute)
  • Civi-first boot for all UFs (already uses absolute)

@eileenmcnaughton
Copy link
Contributor Author

Just FYI - I was looking at consistency within drupal - ie it is set to full path elsewhere on my drupal install - eg. the drush file, civicrm.config.php, Preboot.civi.setup.php

@totten
Copy link
Member

totten commented Jul 27, 2023

On r-technical impact -- I think this is OK, though it's theoretically possible for some customization/integration to be affected. The important thing is that the contract is still basically compatible.

  • Yesterday, a robust consumer would need to allow for relative or absolute values.
  • Today, a robust consumer should still allow for relative or absolute values. (We haven't thoroughly combed through all possible environments...)
  • Tomorrow (or 4 months from now... or somesuch...), as it gets more consistent, consumers can start to assume that values are absolute.

@totten totten merged commit 403c398 into civicrm:7.x-master Jul 27, 2023
@eileenmcnaughton eileenmcnaughton deleted the setting_path branch July 27, 2023 01:23
@demeritcowboy
Copy link
Contributor

Is this the same issue as #667?

@totten
Copy link
Member

totten commented Jul 27, 2023

They sound different to me, eg

  • In 667, it sounds like CIVICRM_SETTINGS_PATH is defined twice (in the same use-case/process/request).
  • In 668, it sounds like CIVICRM_SETTINGS_PATH is defined once - but the value can be different (depending on the use-case/process/request)

wmfgerrit pushed a commit to wikimedia/wikimedia-fundraising-crm that referenced this pull request Jul 27, 2023
https: //github.com/civicrm/civicrm-drupal/pull/668
Change-Id: Id64319091cea495d6a98d09aba0239a1dda925b5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants