8000 Set $_SERVER['SERVER_SOFTWARE'] to an empty string rather than NULL by michaelmcandrew · Pull Request #231 · civicrm/cv · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Set $_SERVER['SERVER_SOFTWARE'] to an empty string rather than NULL #231

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jan 21, 2025
Merged

Set $_SERVER['SERVER_SOFTWARE'] to an empty string rather than NULL #231

merged 2 commits into from
Jan 21, 2025

Conversation

michaelmcandrew
Copy link
Contributor

When using cv in WordPress, we receive deprecation warnings along the lines of

[PHP Deprecation] str_contains(): Passing null to parameter #1 ($haystack) of type string is deprecated at /var/www/html/wp-includes/vars.php:120
[PHP Deprecation] str_contains(): Passing null to parameter #1 ($haystack) of type string is deprecated at /var/www/html/wp-includes/vars.php:120
[PHP Deprecation] str_contains(): Passing null to parameter #1 ($haystack) of type string is deprecated at /var/www/html/wp-includes/vars.php:127
[PHP Deprecation] str_contains(): Passing null to parameter #1 ($haystack) of type string is deprecated at /var/www/html/wp-includes/vars.php:134
[PHP Deprecation] str_contains(): Passing null to parameter #1 ($haystack) of type string is deprecated at /var/www/html/wp-includes/vars.php:134
[PHP Deprecation] str_contains(): Passing null to parameter #1 ($haystack) of type string is deprecated at /var/www/html/wp-includes/vars.php:141
[PHP Deprecation] str_contains(): Passing null to parameter #1 ($haystack) of type string is deprecated at /var/www/html/wp-includes/vars.php:141

This is because we have set $_SERVER['SERVER_SOFTWARE'] to NULL and WordPress expects it to be a string.

Given what the PHP manual say about the $_SERVER value here: https://www.php.net/manual/en/reserved.variables.server.php, I think this is a reasonable assumption.

$_SERVER is an array containing information such as headers, paths, and script locations. The entries in this array are created by the web server, therefore there is no guarantee that every web server will provide any of these; servers may omit some, or provide others not listed here [...] When running PHP on the command most of these entries will not be available or have any meaning.

[...]

'SERVER_SOFTWARE'
Server identification string, given in the headers when responding to requests.

I'm not sure why we are setting it to NULL specifically but I am guessing it is because some code expects it to be set and that NULL seemed like a reasonable value to set it to. And Drupal does something similar here https://github.com/drupal/drupal/blob/7.x/includes/bootstrap.inc#L643-L651 so maybe that was the inspiration?

This PR is based on the assumption that setting it to an empty string is more 'correct' and that any downstream code is not relying on it being NULL (which I think is a reasonable assumption.

It supercedes #210

@totten
Copy link
Member
totten commented Jan 17, 2025 •
edited
Loading

Looks like a good idea @michaelmcandrew.

I'll look into why the tests are unhappy.

@totten
Copy link
Member
totten commented Jan 18, 2025

civibot, test this please

@totten
Copy link
Member
totten commented Jan 18, 2025 •
edited
Loading

OK, so I think some of the noise in the test-results is fixed. However, comparing last run with recent baseline, it does look like D7 is unhappy with the ''.

I'm not sure why we are setting it to NULL specifically but I am guessing it is because some code expects it to be set and that NULL seemed like a reasonable value to set it to. And Drupal does something similar here https://github.com/drupal/drupal/blob/7.x/includes/bootstrap.inc#L643-L651 so maybe that was the inspiration?

Yeah, I'm pretty sure the idea came from Drupal-land (that file or some similar one, like drush).

This PR is based on the assumption that setting it to an empty string is more 'correct' and that any downstream code is not relying on it being NULL (which I think is a reasonable assumption.

Yeah, I'm not sure either. (Obviously there's no Apache/1.2.3 or nginx/1.2.3 in CLI environment... one could make a case for cv/1.2.3 -- eg 'cv/' . \Civi\Cv\Application::version() -- as being more useful. But it's so much happenstance on what the the different systems expect).

Guess: maybe the D7 jobs are failing on an ornery bit like this (where NULL and '' evaluate differently):

https://github.com/drupal/drupal/blob/7.103/includes/bootstrap.inc#L3817-L3822

@totten totten force-pushed the server-software-empty-string branch from 2dda24d to a456745 Compare January 18, 2025 00:49
@totten
Copy link
Member
totten commented Jan 18, 2025

@michaelmcandrew Tests are happy now.

Let me know if this revision works for you.

(Aside: I'm pretty sure I've seen those cv-wp warnings before, so very glad there's a fix. But I'm having trouble provoking them right now. I'll take your word if it seems good.)

@michaelmcandrew
Copy link
Contributor Author
michaelmcandrew commented Jan 20, 2025 •
edited
Loading

@totten - looks good to me - thanks!!

@totten totten merged commit d77dd03 into civicrm:master Jan 21, 2025
1 check passed
@michaelmcandrew
Copy link
Contributor Author

just to say that this has filtered through to our tools now - I'm enjoying the silence :)

@totten
Copy link
Member
totten commented Jan 30, 2025

Woot! (Hopefully, an exclamation mark doesn't break the silence too much. 😉 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@michaelmcandrew @totten
0