[core] Implemented feature to read config from environment variables

[Bockiii]

Greetings,

First draft of the feature for #2099 .

This will look for environment variables with the naming scheme "RSS_AUTHENTICATION_USERNAME=bob" (Prefix_Section_Key=value) and replace the configuration values with the value from the env variable. It is setup to be the third (and currently last) point in the configuration loader, after the default and then the custom .ini. This means: Env variable will beat default and custom file!

I assume this can be made prettier and/or shorter, but this is the first shot and it's readable.

What I have tested:

• Boolean values get recognized
• Added values (eg RSS_SOMETHING_STUPID=TOKTOK) do not cause errors and if they are not picked up later, dont matter.
• Misformed values will be picked up by the quality checking in the config (e.g: putting in "RSS_AUTHENTICATION_ENABLE=yesplease" will throw the error that its not a valid boolean value)
• Backwards compatibility should be 100% since this only overwrites config items if you define the specific item to be overwritten.

During testing I found some odd behavior that needs to be checked, but this is not down to the config but the general setup. e.g: You can enable authentication, set a password and then dont set a username. rss bridge will still work (leave the user name empty and put in the password). It does not work the other way around though (username but no password). But as I said... nothing to do with this implementation.

[Bockiii]

You could go in and check if there is a second and third (and also not a fourth, fifth etc) array member in order to not import things like "RSS_AUTHENTICATION=Yes", but since this does not seem to cause any errors (you can import that to config and it doesnt mind), I didnt do it for the first shot. It might make sense for the sake of security.

[Bockiii]

I need reviewers on this as well btw. I will adapt the dockerfile to the new multiarch setup, but that wont change anything about this functionality. @captn3m0 you?

[em92]

Hello, @Kwbmm! We discussed this in #2071 (comment)

Could you review it, please?

[captn3m0]

Minor suggestion: changing RSS to RSSBRIDGE in favor of being explicit.

[Bockiii]

Doesn't that just needlessly inflate the config?
RSSBRIDGE_AUTHENTICATION_USERNAME is already quite a handful.
I would argue for the other end :
RB_AUTHENTICATION_USERNAME

Since the env variables are for this single container anyways, I don't see a value in making the variable longer.

[Kwbmm]

Looks good to me. I've personally implemented something slightly different for authenticating on my fork here: 4c26918 (and a fix for the spelling mistake here: 0dbe21b). The idea is that each section of the config file can have a from_env key and if set to true then you'll read the vars (for that section) from the environment.

But this works as well I guess. :)

EDIT: I'm no php programmer here, so I'll let others get into the details of the implementation. Can't really tell if it's better to read all envs or read just when the section in the config specifies so. Maybe with too many env variables this implementation gets heavy? 🤷

[Bockiii]

Even if you add 50 env vars, it's going to loop for about 70 times (because off the default env vars). That should take php about 0.7 seconds :) so it's all good.

This way you also don't have to change your config at all. It's all in the env.

[Bockiii]

So current request would be "Discuss if it should be "RSS" or "RSSBRIDGE" (I'll retract my "RB" since I dont even like that myself).

I am for RSS but I'm also not against the RSSBRIDGE version. In my opinion it's just not necessary to write it out.

Someone decide, I will change it to whatever @em92 or so wants. I think the technical part is working, its just a question of which prefix to use.

[Bockiii]

I'm not sure on how to fix the issue that the test has.

@em92 ?

[Bockiii]

@em92 this will also fix the heroku build problem in #2159 . Another solution would be to implement #2098

[Bockiii]

@em92 it seems like I cant use the CONFIGURATION:: call there. Can you fix the code so I can pull from the config in that if-clause?

Other than that, it should be mergeable. This will fix a bunch of deployment issues (or rather: This can be the base for a fix for many deployment issues)

[em92]

Hi, @Bockiii!

• I have removed whitelistall config because (1) it is not related to this PR (2) original PR [BridgeFactory.php] Add RSSBRIDGE_WHITELIST environment variable support #2058 allows to list certain bridge names, while "whitelistall" does not.

• There is a blocker https://github.com/RSS-Bridge/rss-bridge/pull/2100/checks?check_run_id=3174948101 which made me to drop php7.0- support. I started [core] Drop php 5.6 and php 7.0 support #2224. When that PR is ready, I will merge this PR.

[em92]

it seems like I cant use the CONFIGURATION:: call there. Can you fix the code so I can pull from the config in that if-clause?

Why do you need it?

[Bockiii]

With the removed whitelistall functionality, I don't need it anymore.

It was an extention to the idea of configuring rss ridge through environment variables, that's why I included it here. But if you don't wants that, no problem.

[em92]

Hey, @Bockiii! Please pull my changes in this PR and rebase on latest master. We just dropped 5.6 support, so automatic tests should be green now.

[Bockiii]

@em92 I dont think there needs to be anything done right? Your changes are in (namely the removal of the whitelist and the rename to "RSSBRIDGE") and all checks passed. I think you can merge this.

[em92]

Actually you are right!