8000 [Security] Implement fluent interface on RememberMeBadge::disable() by derrabus · Pull Request #41787 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Security] Implement fluent interface on RememberMeBadge::disable()#41787

Merged
derrabus merged 1 commit intosymfony:5.3from
derrabus:bugfix/remember-me-fluent-disable
Jun 22, 2021
Merged

[Security] Implement fluent interface on RememberMeBadge::disable()#41787
derrabus merged 1 commit intosymfony:5.3from
derrabus:bugfix/remember-me-fluent-disable

Conversation

@derrabus
Copy link
Member
@derrabus derrabus commented Jun 22, 2021
Q A
Branch? 5.3
Bug fix? yes
New feature? no
Deprecations? no
Tickets N/A
License MIT
Doc PR N/A

I was a bit puzzled when a I saw that RememberMeBadge::enable() implements a fluent interface while its counterpart disable() does not. This PR fixes this inconsistency.

The class is flagged as @final, so this change should not violate the BC promise.

@derrabus
Implement fluent interface on RememberMeBadge::disable()
d12f76f 
Signed-off-by: Alexander M. Turek <me@derrabus.de>
@derrabus derrabus requested review from chalasr and wouterj as code owners June 22, 2021 13:40
@carsonbot carsonbot added this to the 5.3 milestone Jun 22, 2021
@wouterj
Copy link
Member
wouterj commented Jun 22, 2021 •
edited
Loading

While I'm OK with merging, is there a real use-case for disable being fluent? I mean, $badge->disabled()->isEnabled() doesn't make much sense, and there aren't any other methods in the badge?

The reason btw for enabled() being fluent is because the default is disabled. I like doing (new RememberMeBadge())->enabled() instead of having to use a variable.

@derrabus
Copy link
Member Author
derrabus commented Jun 22, 2021

I made this PR mainly to create a consistent behavior. The only use-case I can think of is a function that mutates and returns the badge, e.g.

return $badge->disable();

instead of

$badge->disable();

return $badge;

wouterj
wouterj approved these changes Jun 22, 2021
View reviewed changes
Copy link
Member
@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

Makes sense!

@derrabus derrabus merged commit 8dfdd89 into symfony:5.3 Jun 22, 2021
@fabpot fabpot mentioned this pull request Jun 30, 2021
Merged
@derrabus derrabus deleted the bugfix/remember-me-fluent-disable branch November 21, 2021 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@javiereguiluz javiereguiluz javiereguiluz approved these changes

@wouterj wouterj wouterj approved these changes

@chalasr chalasr chalasr approved these changes

Assignees

No one assigned

Labels

Bug Security Status: Reviewed

Projects

None yet

Milestone

5.3

Development

Successfully merging this pull request may close these issues.

5 participants

@derrabus @wouterj @javiereguiluz @chalasr @carsonbot
0