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

Properly set Pantheon environment variables: $_SERVER['BACKDROP_SETTINGS']['settings'] should be set instead of $_SERVER['BACKDROP_SETTINGS']['conf'] #58

Closed
herbdool opened this issue Mar 21, 2022 · 10 comments · Fixed by #64 · May be fixed by #60

Comments

@herbdool
Copy link
Contributor

herbdool commented Mar 21, 2022

$_SERVER['BACKDROP_SETTINGS'] is set from $_SERVER['PRESSFLOW_SETTINGS'] but that is the old d7 variable. There's backdrop/backdrop#60 which added support for this.

Without this, we can't use settings_get(). With this it makes it easier to port pantheon_apachesolr module from drop7.

@herbdool herbdool changed the title $_SERVER['BACKDROP_SETTINGS']['conf'] should be settings $_SERVER['BACKDROP_SETTINGS']['settings'] should be set instead of $_SERVER['BACKDROP_SETTINGS']['conf'] Mar 21, 2022
@herbdool
Copy link
Contributor Author

herbdool commented Mar 21, 2022

@joelsteidl or @eric2pin wondering if either of you would like to test this. I was porting pantheon_apachesolr and noticed it expected the environment variables in variable_get (now settings_get()).

Not quite sure the best way to merge the settings. I'm thinking that since $_SERVER['BACKDROP_SETTINGS'] is being set the very end it shouldn't override settings higher up in the file. Alternatively, the whole "Pantheon specific compatibility" could be moved to near the top then it might be easier for people to follow the flow.

Anything in $_SERVER['BACKDROP_SETTINGS'] (settings index) overrides what is set in $settings in bootstrap.inc.

@eric2pin
Copy link

@herbdool Happy to help but could use more direction on how to help. Are you just trying to make sure that your changes to settings.php and the way $_SERVER['BACKDROP_SETTINGS'] is set, which help with the pantheon_apachesolr port, still works as expected with the Pantheon env?

@herbdool
Copy link
Contributor Author

@eric2pin yup. I don't think there are any issues with the PR but it would help to have a second set of eyes. Since yesterday I realized that there isn't likely anything that's being added to $settings global with this PR will change anything in Backdrop out of the box. Even $settings['file_temporary_path'] won't do anything since the system is only checking config files. (Separately I've got #34 as well to actually set the temp path.)

@herbdool herbdool changed the title $_SERVER['BACKDROP_SETTINGS']['settings'] should be set instead of $_SERVER['BACKDROP_SETTINGS']['conf'] Properly set Pantheon environment variables: $_SERVER['BACKDROP_SETTINGS']['settings'] should be set instead of $_SERVER['BACKDROP_SETTINGS']['conf'] Mar 22, 2022
@herbdool
Copy link
Contributor Author

@eric2pin there were a few changes to settings to be made so I figured it would be better to put the separate PRs together into one branch #64 and then merge that into master. That will reduce the number of updates downstream people have to deal with. In the PR it links to which issues are addressed.

Note that I've proposed the creation of settings.pantheon.php to match drop8 file of the same name: https://github.com/pantheon-systems/drops-8/blob/default/sites/default/settings.pantheon.php.

@eric2pin
Copy link

eric2pin commented Mar 29, 2022

@herbdool I pulled this into a Pantheon dev env clone of one of our production sites and just pasted in the following so my site could still run:

if (file_exists(DIR . '/settings.my_site_settings.php')) {
include DIR . '/settings.my_site_settings.php';
}

In the future I could easily change our settings file to settings.local.php so no changes would be necessary.

So far I see that $_ENV['HASH_SALT'] is undefined. It looks like the index DRUPAL_HASH_SALT is defined and appears to be the salt value we're looking for. I suppose this could be something to do with my Pantheon config but I did spin up the site based on the Backdrop CMS Pantheon upstream.

I'll continue poking around, but otherwise seems fine so far.

@herbdool
Copy link
Contributor Author

I spun up a new site using the Backdrop Pantheon upstream too and when I look at $ENV I see HASH_SALT but not DRUPAL_HASH_SALT (unlike in drop9 where it does seem to exist). It's odd that you're seeing the opposite.

There is a drupal_hash_salt in $_SERVER['BACKDROP_SETTINGS'].

Just to be sure I made sure to create the site by going to backdropcms.org/demo

@eric2pin
Copy link

You're correct, when I spin up from the Backdrop CMS upstream link, I get the salt in the HASH_SALT index. The sites I used as a base to test are a custom upstream (Backdrop CMS upstream + our custom code) and we probably selected "Drupal" as the framework somewhere along the way in Pantheon's custom upstream setup.

@herbdool
Copy link
Contributor Author

Thanks @eric2pin. In that case, I'm leaning towards not addressing that edge case and assume that people are using the main approach. Though it wouldn't be hard to check if either are set. What do you think?

Just to be sure, what you're seeing is $_ENV['DRUPAL_HASH_SALT']?

@eric2pin
Copy link

Yes, what I'm seeing is $_ENV['DRUPAL_HASH_SALT']. Hard to say -- we're almost certainly an edge case but Pantheon itself for Backdrop CMS is in edge case territory as far as I know. It's super easy to check for both -- maybe until Pantheon has its own option for Backdrop CMS out-of-the-box it doesn't hurt to cover both options.

@herbdool
Copy link
Contributor Author

Thanks @eric2pin I've updated it so it checks for both. Let me know if you notice anything else. Otherwise I think it's ready to merge.

herbdool added a commit that referenced this issue Apr 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants