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

[HttpFoundation] added withers to Cookie class #35215

Open
wants to merge 4 commits into
base: master
from

Conversation

@ns3777k
Copy link

ns3777k commented Jan 4, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #35212
License MIT
Doc PR -

I was quite descriptive in the issue :-)

The main idea is to get the interface for changing a cookie to avoid every unneeded argument in the constructor.

Current:

$cookie = Cookie::create(
    RegionSwitcher::REGION_COOKIE, $regionSlug, new DateTime('+1 year'), '/',
    $baseDomain, null, false
);

This PR:

$cookie = Cookie::create('foo')
            ->withValue('bar')
            ->withExpiresTime(strtotime('Fri, 20-May-2011 15:25:52 GMT'))
            ->withDomain('.myfoodomain.com')
            ->withSecure(true);

Every wither returns a copy of current cookie with requested setting set. Cookie class remains immutable.

@Taluu

This comment has been minimized.

Copy link
Contributor

Taluu commented Jan 4, 2020

I'm kinda against having a fluent interface as it doesn't really make sense to have a fluent here (it's not a builder...)

Adding a mutable state why not, and maybe keep it immutable (returning a new instance with the property set correctly) would be even better IMO.

(Hence my 😕 / 👎 reaction)

@ns3777k

This comment has been minimized.

Copy link
Author

ns3777k commented Jan 4, 2020

@Taluu Maybe fluent interface is not correct term here, I meant a bunch of setters by it just to mutate the state (that's exactly what's been done.).

Sorry for my bad english but what's the difference between this PR and Adding a mutable state why not?

@Taluu

This comment has been minimized.

Copy link
Contributor

Taluu commented Jan 4, 2020

You made it fluent (returning $this) is the key difference here. :)

@ns3777k

This comment has been minimized.

Copy link
Author

ns3777k commented Jan 4, 2020

Oh, I got you :-) I don't mind changing the PR so setters would become something like:

public function setName(string $name): self
{
    // ...validating $name...
    $clone = clone $this;
    $clone->name = $name;

    return $clone;
}

But I personally prefer $this :-)
Anyway, let's wait on other opinions.

@Taluu

This comment has been minimized.

Copy link
Contributor

Taluu commented Jan 5, 2020

Even better : make withers (withName...).

Or remove the return and make it mutable.

@nicolas-grekas nicolas-grekas added this to the next milestone Jan 5, 2020
@ns3777k ns3777k force-pushed the ns3777k:ticket_35212 branch from b02d031 to 3eb2be6 Jan 5, 2020
@ns3777k

This comment has been minimized.

Copy link
Author

ns3777k commented Jan 5, 2020

@Taluu

I updated the PR with withers keeping the Cookie immutable. Appreciate it if you take a look.

@ns3777k ns3777k changed the title [HttpFoundation] added fluent interface for Cookie class [HttpFoundation] added withers to Cookie class Jan 6, 2020
@ns3777k ns3777k force-pushed the ns3777k:ticket_35212 branch from 3eb2be6 to b80a2ae Jan 6, 2020
@florianajir

This comment has been minimized.

Copy link

florianajir commented Jan 7, 2020

I need this ! but sad to have to wait for 5.2.0 release to get mutable cookie :/

src/Symfony/Component/HttpFoundation/Cookie.php Outdated Show resolved Hide resolved
src/Symfony/Component/HttpFoundation/Cookie.php Outdated Show resolved Hide resolved
src/Symfony/Component/HttpFoundation/Cookie.php Outdated Show resolved Hide resolved
src/Symfony/Component/HttpFoundation/Cookie.php Outdated Show resolved Hide resolved
src/Symfony/Component/HttpFoundation/Cookie.php Outdated Show resolved Hide resolved
src/Symfony/Component/HttpFoundation/Cookie.php Outdated Show resolved Hide resolved
src/Symfony/Component/HttpFoundation/Cookie.php Outdated Show resolved Hide resolved
src/Symfony/Component/HttpFoundation/Cookie.php Outdated Show resolved Hide resolved
@nicolas-grekas nicolas-grekas force-pushed the ns3777k:ticket_35212 branch 2 times, most recently from a734471 to d7bc008 Jan 12, 2020
-
@nicolas-grekas nicolas-grekas force-pushed the ns3777k:ticket_35212 branch from d7bc008 to c6e60e4 Jan 12, 2020
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 12, 2020

Thanks for your explanations @ns3777k, I missed that some attributes should remain nullable.
I pushed a third commit on your fork with additional changes.
As a last step, I think we should group all wither methods at the beginning of the class, before the getters.

@ns3777k

This comment has been minimized.

Copy link
Author

ns3777k commented Jan 12, 2020

Thanks for clarification! @nicolas-grekas I'm gonna move the withers up.

@ns3777k ns3777k force-pushed the ns3777k:ticket_35212 branch from 69fb08b to 8ae8fbb Jan 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.