Closed joesaunderson closed 3 years ago
@Twixes I assume you're the Posthog PHP guy? What is your opinion on this one?
Can't claim to be a PHP guy. 😂 With PHP I only do housekeeping. As for this idea, we are all for env vars at PostHog, but for libraries we'd like to avoid using them directly – instead we'd like library users to specify settings via the PostHog library's API in the app. So that way config setup is purely up to you, or the framework you use e.g. Symfony, without the library going around it using env vars. I'll close this, but thank you for putting this up for consideration.
Can't claim to be a PHP guy. 😂 With PHP I only do housekeeping. As for this idea, we are all for env vars at PostHog, but for libraries we'd like to avoid using them directly – instead we'd like library users to specify settings via the PostHog library's API in the app. So that way config setup is purely up to you, or the framework you use e.g. Symfony, without the library going around it using env vars. I'll close this, but thank you for putting this up for consideration.
I mostly agree with this, that libraries should be framework agnostic. But this PR supported both ways, it would continue to support:
PostHog::init("<ph_project_api_key>",
array('host' => '<ph_instance_address>') // You can remove this line if you're using app.posthog.com
);
but would also support (with env vars):
PostHog::init();
This PR simply removes the need to get from the environment variables, with no breaking changes whatsoever.
Okay, I'd be inclined to approve this PR if it'd have a single source for all POSTHOG_
settings. The problem is that with each class getting its setting itself, each class effectively has its own settings-from-env mechanism, which is not good from a maintenance perspective. If $apiKey
and $options
were overridden from env at the top level exposed to library users, that'd be much nicer and easier to keep track of.
Okay, I'd be inclined to approve this PR if it'd have a single source for all
POSTHOG_
settings. The problem is that with each class getting its setting itself, each class effectively has its own settings-from-env mechanism, which is not good from a maintenance perspective. If$apiKey
and$options
were overridden from env at the top level exposed to library users, that'd be much nicer and easier to keep track of.
Do you mean a single file of the constants i.e PosthogEnvConstants
or only one class where we read from the env variables?
Just a single function overrideConfigFromEnv
, which would do the overriding for both $apiKey
and $options
in one place (PostHog
constructor seems like the right place to run this).
Makes sense to me, I will action this and put it up for another review.
Pushed out 2.0.1 with this.
Nice, thanks. What's the right docs repo to update for the website?
This file would be the right one: https://github.com/PostHog/posthog.com/blob/master/contents/docs/libraries/php.mdx
Notes
Libraries such as Symfony heavily make use of environment variables and encourage their usage. This PR allows for the two major settings in the PHP implementation (api key, and host) to be setup using environment variables. The major benefit of this is allowing different configuration across environments (local, staging and prod for example).
This has no breaking changes, if the environment variables are set it will use them, if not, it will default to the previous methods.
Before:
After:
I think this could also do with a PR to update the docs at https://posthog.com/docs/libraries/php, but I wanted to validate this PR first...