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

If context is defined, it is used instead of the firewall #5981

Closed
wants to merge 1 commit into from

Conversation

ruslan-fidesio
Copy link
Contributor

This should be mentioned.

@@ -45,7 +45,7 @@ with a request. The following example demonstrates this technique::
{
$session = $this->client->getContainer()->get('session');

$firewall = 'secured_area';
$firewall = 'secured_area'; // put here firewalls context if defined
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can expand this a bit and put a comment like the following above this line:

// the name of the firewall to authenticate too (replace this with the value
// of the "context" option if it was used in the firewall's configuration)

What do you think?

@wouterj
Copy link
Member

wouterj commented Jul 9, 2016

Hi @ruslan-fidesio! I like how you explained this variable, which is not clear at all from the code example.

As the firewall context defaults to the firewall name, I think we can simply say that this is the firewall context. I've taken your commit and added a new comment, using the new wordings in #6740 This way, you still get the credits for adding this nice clarifation.

Thanks for starting this!

@wouterj wouterj closed this Jul 9, 2016
wouterj added a commit that referenced this pull request Jul 9, 2016
…le (ruslan-fidesio, WouterJ)

This PR was merged into the 2.7 branch.

Discussion
----------

Add little comment indicating meaning of $firewall variable

Finishes #5981

Commits
-------

2a702c3 Improve comment
2545203 If context is defined, it is used instead of the firewall
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.

3 participants