[Bug fix] Allow to disable authorize #466
Conversation
vincentchalamon
commented
May 18, 2017
vincentchalamon
commented
| Q | A |
|---|---|
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #465 |
| License | MIT |
| } | ||
|
|
||
| $this->loadAuthorize($config['authorize'], $container, $loader); |
There was a problem hiding this comment.
Isn't this change doing the exact same thing? How does it work?
There was a problem hiding this comment.
It's not the same thing as my version exit the function, so lines 99->104 won't be executed. In original version, they are executed even if authorize feature is disabled, which is a bug IMO.
Still waiting for the team to review & merge this PR
There was a problem hiding this comment.
Version 1.6 is about to be tagged soon. We may be able to squeeze this in.
I think you would have better chance of this getting thru if you solve the conflicts. Unit testing the change would be nice too.
There was a problem hiding this comment.
Conflicts solved, but this class is not unit tested
There was a problem hiding this comment.
You are right. I was mislead by the presence of the test file.
vincentchalamon
force-pushed
the
master
branch
from
3ebee0f to
9bfe6aa
Compare
|
@dkarlovi, could we have this in the milestone for 1.6? |
|
please merge it! |
|
Fixed in #554. |
