Skip to content

Documented the arguments of the Cookie class #6441

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

Closed
wants to merge 5 commits into from

Conversation

javiereguiluz
Copy link
Member

Q A
Doc fix? no
New docs? yes
Applies to 2.3+
Fixed tickets -

``$expire``
**type**: ``int``|``string``|``DateTime``|``DateTimeInterface`` **default**: 0
The time the cookie expires. This value can be set as a timestamp integer,
as a ``strtotime()`` valid date string (e.g. ``+1 week``), as a ``DateTime``
Copy link
Contributor

Choose a reason for hiding this comment

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

For convenience, you could link the strtotime docs in here. This makes it a lot easier to find the possible values for those who are unfamiliar with it

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. Done. Thanks.

takes eight arguments in its constructor:

``$name``
**type**: ``string`` **default**: none (this argument is mandatory)
Copy link
Member

Choose a reason for hiding this comment

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

There always needs to be a blank line after the first line of the explanation (otherwise we would have only one paragraph for each explanation).

Copy link
Member Author

Choose a reason for hiding this comment

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

I've done that and I've improved the explanation of the $httpOnly argument.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should also add a blank line before we describe the next term.

Copy link
Member Author

Choose a reason for hiding this comment

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

Much better. Done.

@@ -344,7 +344,47 @@ attribute::
The
:method:`Symfony\\Component\\HttpFoundation\\ResponseHeaderBag::setCookie`
method takes an instance of
:class:`Symfony\\Component\\HttpFoundation\\Cookie` as an argument.
:class:`Symfony\\Component\\HttpFoundation\\Cookie` as an argument. This class
takes eight arguments in its constructor:
Copy link
Member

Choose a reason for hiding this comment

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

seven

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Only Symfony 3.1 defines 8. Fixed. Thanks!

@xabbuh
Copy link
Member

xabbuh commented Apr 11, 2016

👍

@weaverryan
Copy link
Member

Hi @javiereguiluz!

I'm not totally sure about it. My ideal situation would be that things like this is handled by

A) Added to the PHP doc in the class itself (as it is, this is duplication of the PHPDoc, plus a little bit more details)
AND
B) Including a short usage example with a link to the PHP docs

Wdyt? I know Fabien historically doesn't love php documentation, but I want to minimize maintenance, and we could (in theory) document every argument to every class on the docs (which would be a lot of duplication).

Thanks!

@wouterj
Copy link
Member

wouterj commented Apr 18, 2016

I agree with @weaverryan here. This documentation seems out of place for the docs (why document these arguments in such a detail, but don't do it for all other arguments for instance). The only two places where we do such things are with twig functions/filters and config options. These have no clear way to document them using doc blocks.

So, because 2 documentors voted against this, I'm going to close this one. Feel free to comment/reopen if you feel it's necessary.

@wouterj wouterj closed this Apr 18, 2016
@javiereguiluz
Copy link
Member Author

@wouterj I like a lot the idea of reducing the maintenance burden for the docs! However, arguments are already explained in lots of places in the Symfony Docs. Should we remove all that information too?

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.

5 participants