[Security] improve some firewall config comments #20404
Conversation
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #19398 |
| License | MIT |
| Doc PR |
| * @param string|null $entryPoint | ||
| * @param string|null $accessDeniedHandler | ||
| * @param string|null $accessDeniedUrl | ||
| * @param string $userChecker |
There was a problem hiding this comment.
Isn't it a bit strange to have this mandatory argument after all those optional nullable arguments?
There was a problem hiding this comment.
Yes indeed. We could still reorder the arguments.
There was a problem hiding this comment.
we should indeed reorder them before being stuck with BC issues
|
The SecurityDataCollectorTest needs to be updated due to the argument becoming mandatory. If you want, I can do this change and btw reorder the arguments, so you just correct the doc here. |
| */ | ||
| public function getAccessDeniedHandler() | ||
| { | ||
| return $this->accessDeniedHandler; | ||
| } | ||
|
|
||
| /** | ||
| * @return string|null The access_denied_handler service id if configured, null otherwise |
| * @param string|null $entryPoint | ||
| * @param string|null $accessDeniedHandler | ||
| * @param string|null $accessDeniedUrl | ||
| * @param string $userChecker |
There was a problem hiding this comment.
we should indeed reorder them before being stuck with BC issues
|
@chalasr I reverted the change to not allowing |
|
Thank you @xabbuh. |
This PR was merged into the 3.2-dev branch. Discussion ---------- [Security] improve some firewall config comments | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #19398 | License | MIT | Doc PR | Commits ------- cb6c703 [Security] improve some firewall config comments
… mandatory (chalasr) This PR was merged into the 3.2-dev branch. Discussion ---------- [SecurityBundle] FirewallConfig's user_checker should be mandatory | Q | A | ------------- | --- | Branch? | master | Tests pass? | yes | Fixed tickets | #20404 (comment) | License | MIT Commits ------- 6754af2 [SecurityBundle] FirewallConfig's user_checker should be mandatory