[RTM] Feature/trusted proxy support

[Toflar]

This is a PR for #128. I checked the code and imho our legacy code does exactly what Symfony's Request::getClientIp() does.

Please double check this though :)

[aschempp]

I've noticed that too in https://github.com/contao/core-bundle/pull/72/files#diff-2f0eb5d18e0f5fb0dbaef526bb165d41R305

Should we add the TL_CONFIG variable for BC? We currently do that for all other variables, and I do have some customization where I use the variable.

[Toflar]

Should we add the TL_CONFIG variable for BC? We currently do that for all other variables, and I do have some customization where I use the variable.

BC of what? What would it help to add the variable?

[aschempp]

I mean adding it here:
https://github.com/contao/core-bundle/blob/develop/src/Resources/contao/library/Contao/Config.php#L484-L505

[Toflar]

But what does it help? It sets a TL_CONFIG value that is never used.

[aschempp]

We don't know if extension developers used it. Same with SMTP, it's unlikely someone used it because the Email class does automatically use it, but it would not harm to add it for the sake of completeness.

[Toflar]

But it's not really possible because we did not support all the notations IpUtils supports. One can configure an IP range in the trusted_proxy settings and we would need to resolve this to a list of comma separated IP's to set that value. I mean, of course it's possible but is it worth the effort?

[aschempp]

That's true, assuming people will start using IP ranges…

[leofeyer]

Looks good to me. Let me know when it is RTM :)

[Toflar]

Okay for you and @aschempp so it's RTM for me :)

[leofeyer]

Merging this will invalidate #128, right?

[Toflar]

Not invalidate but close :-)

[aschempp]

We should also close my PR in contao/core for ip-range support and mention this

[leofeyer]

Can you fix the tests please?

PHP Fatal error:  Call to a member function getContainer() on null in /var/www/contao/standard-edition/vendor/contao/core-bundle/src/Resources/contao/library/Contao/Environment.php on line 376

The method is probably missing in our kernel mock.

[aschempp]

global $kernel is missing ;)

[Toflar]

Fixed. But now I have a new problem that's probably caused by some component overriding the $_SERVER or whatever. I can't find the issue...

[aschempp]

I guess here's the problem:
https://github.com/contao/core-bundle/blob/develop/tests/TestCase.php#L96

[Toflar]

True, why is this?

[aschempp]

I guess because a default value is alway necessary…

Maybe we should change that to something like this:

if (empty($_SERVER['HTTP_ACCEPT_LANGUAGE'])) {
    $_SERVER['HTTP_ACCEPT_LANGUAGE'] = '';
}

[leofeyer]

I have no idea. I think the mockKernel() method is from @discordier?

[leofeyer]

Do the tests still work if you remove the line?

[discordier]

Having similar issues in my pr. Stay tuned. Will search a solution tomorrow.

[leofeyer]

I have fixed the tests and merged the PR in c375b7c.