Skip to content
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.

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

[SecurityBundle] BasicAuthenticationListener: simpler getting value from Request #19499

Merged
merged 1 commit into from
Aug 23, 2016

Conversation

MacDada
Copy link
Contributor

@MacDada MacDada commented Aug 1, 2016

Q A
Branch? 2.7
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

Unless I'm mistaken, the default null should be OK. If it's not, I will create a new PR with a test proving that false or other "special" value must be used.

…t on getting a header value

it's unnecessary.
@ogizanagi
Copy link
Contributor

I don't know if this can actually happen in real life, but those changes won't differentiate anymore the PHP_AUTH_USER header being null from being absent. Which is probably the reason why it was written as it is (Of course, I agree the situation would be weird).

Otherwise 👍

@MacDada
Copy link
Contributor Author

MacDada commented Aug 4, 2016

those changes won't differentiate anymore the PHP_AUTH_USER header being null from being absent

@ogizanagi Yep. But before changes u cannot differentiate PHP_AUTH_USER header being false from absent xD

So, the real question is: what kind of values can never be there, so that we can treat them as "no values". null seems to be a good default one, unless it's somehow special here. I don't know. Tests pass. PR is open ;-)

@fabpot
Copy link
Member

fabpot commented Aug 15, 2016

What's the benefit of this change?

@MacDada
Copy link
Contributor Author

MacDada commented Aug 15, 2016

What's the benefit of this change?

Less WTF while reading the code.

@nicolas-grekas
Copy link
Member

👍

@fabpot fabpot merged commit d67f090 into symfony:2.7 Aug 23, 2016
fabpot added a commit that referenced this pull request Aug 23, 2016
…tting value from Request (MacDada)

This PR was merged into the 2.7 branch.

Discussion
----------

[SecurityBundle] BasicAuthenticationListener: simpler getting value from Request

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| License       | MIT

Unless I'm mistaken, the default `null` should be OK. If it's not, I will create a new PR with a test proving that `false` or other "special" value must be used.

Commits
-------

d67f090 SecurityBundle:BasicAuthenticationListener: removed a default argument on getting a header value
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants