elasticsearch-php
elasticsearch-php copied to clipboard
elastic
7.x - ELASTIC_CLIENT_APIVERSIONING should use $_SERVER and $_ENV instead of getenv to be thread safe with symfony/dotenv (setting the value inside PHP)
Summary of problem or feature request
When using symfony/dotenv to provide enviornment variables, this does not use putenv as putenv is not thread safe https://github.com/symfony/dotenv/blob/v5.4.5/Dotenv.php#L70
However, if one uses getenv instead of $_SERVER and $_ENV the use of putenv is required. Which would not be thread safe. Or it needs to be provided outside PHP but in case of multiple threads using different environment variables it still would not work.
So instead of using getenv (or just getenv), $_SERVER and $_ENV should be used. This way it would also work with symfony/dotenv in default configuration (putenv is disabled).
Normally, $_SERVER['ELASTIC_CLIENT_APIVERSIONING'] ?? $_ENV['ELASTIC_CLIENT_APIVERSIONING'] should be able to target all set environment variables. Optionally, a third fallback could be getenv.
It would be a simple change and it would be thread safe and still works everywhere.
System details
- Operating System
- PHP Version 8.1
- ES-PHP client version 7.17.0
- Elasticsearch version 7.14
@AndreasA I'm sorry but I do not understand your request. The env variable that you mentioned ELASTIC_CLIENT_APIVERSIONING is not present in this project. Moreover elasticsearch-php does not use putenv(). Can you explain better what you mean? Thanks!
Hi @ezimuel if you still support 7.x you do use it https://github.com/elastic/elasticsearch-php/blob/v7.17.0/src/Elasticsearch/ClientBuilder.php#L640
and you do not check $_SERVER and/or $_ENV, so putenv would be necessary to change it using symfony/dotenv.
If not, just a friendly reminder for future such environment variable usages 😄
@AndreasA ok, I misunderstood your point. I'll send a PR to fix avoid the usage of getenv(). Thanks for reporting this issue!
@ezimuel No problem. I think you can could probably leave getenv to query as a last resort but I think checking $_SERVER['ELASTIC_CLIENT_APIVERSIONING'] ?? $_ENV['ELASTIC_CLIENT_APIVERSIONING'] should be enough and would be thread-safe.
@AndreasA I sent the PR #1247 for fixing the issue. I used the following approach:
$apiVersioning = $_SERVER['ELASTIC_CLIENT_APIVERSIONING']
?? $_ENV['ELASTIC_CLIENT_APIVERSIONING']
?? getenv('ELASTIC_CLIENT_APIVERSIONING');
I'm still using getenv() as fallback condition since if someone is using putenv() the only way to get the env variable is using getenv().