8000 [Cache] Document "framework.cache" nodes by geoffrey-brier · Pull Request #7245 · symfony/symfony-docs · GitHub
[go: up one dir, main page]
Skip to content

[Cache] Document "framework.cache" nodes #7245

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

[Cache] Document "framework.cache" nodes #7245

wants to merge 3 commits into from

Conversation

geoffrey-brier
Copy link
Contributor

No description provided.

@geoffrey-brier geoffrey-brier force-pushed the framework-cache-directory branch 3 times, most recently from 3364073 to be1404b Compare December 11, 2016 23:19
@geoffrey-brier geoffrey-brier changed the base branch from 3.2 to 3.1 December 11, 2016 23:20
@geoffrey-brier geoffrey-brier force-pushed the framework-cache-directory branch 2 times, most recently from 75a1819 to 492250f Compare December 11, 2016 23:29
@Jean85
Copy link
Contributor
Jean85 commented Dec 13, 2016

This needs to be fixed, it appears that the first few changed lines are broken.

Status: needs work

@stof
Copy link
Member
stof commented Dec 13, 2016

Al cache settings should be documented instead of documenting only one of them.

@geoffrey-brier
Copy link
Contributor Author
geoffrey-brier commented Dec 13, 2016 •
edited
Loading

@stof Sure I can do it.

Does anybody know why the test fails ? I've been searching for "cache" reference inside the document but couldn't find any @Jean85

Jean85
Jean85 reviewed Dec 13, 2016
View reviewed changes
reference/configuration/framework.rst
@@ -102,6 +102,8 @@ Configuration
* :ref:`cache <reference-serializer-cache>`
* :ref:`enable_annotations <reference-serializer-enable_annotations>`
* :ref:`name_converter <reference-serializer-name_converter>`
* `cache`_
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe here is why it's failing? It's misaligned...

@geoffrey-brier geoffrey-brier changed the title [Cache] Document "framework.cache.directory" option [Cache] Document "framework.cache" nodes Dec 13, 2016
@geoffrey-brier geoffrey-brier force-pushed the framework-cache-directory branch from 492250f to 716f826 Compare December 13, 2016 19:47
@geoffrey-brier geoffrey-brier force-pushed the framework-cache-directory branch 11 times, most recently from 206ccc7 to fdd3b98 Compare December 13, 2016 21:12
xabbuh
xabbuh reviewed Dec 13, 2016
View reviewed changes
reference/configuration/framework.rst
@@ -55,7 +55,7 @@ Configuration
* `session`_
* `storage_id`_
* `handler_id`_
* `name`_
* :ref:`name <session-name>`
Copy link
Member

Choose a reason for hiding this comment

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

What was the reason for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I added another name reference. As this leads to a warning (considered as an error by the build), I figured out that you have rename all your references this way.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't it enough to only do that for the other added label with the same name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested it and it failed ^^

Copy link
Member

Choose a reason for hiding this comment

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

Too bad, this means that the anchor used currently (http://symfony.com/doc/current/reference/configuration/framework.html#name) won't work in the future. Maybe @wouterj has an idea how we can prevent the loss.

Copy link
Member

Choose a reason for hiding this comment

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

@xabbuh adding the .. _name: label manually before the existing name option wouldn't work?

Copy link
Member

Choose a reason for hiding this comment

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

@javiereguiluz Might cause conflicts as these labels are global and not limited to the current document.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@javiereguiluz sounds like your solution worked :)

@geoffrey-brier geoffrey-brier force-pushed the framework-cache-directory branch from fdd3b98 to c1525c0 Compare December 13, 2016 21:33
@geoffrey-brier geoffrey-brier force-pushed the framework-cache-directory branch 4 times, most recently from 86f93e9 to cdf1046 Compare December 16, 2016 17:59
@geoffrey-brier geoffrey-brier force-pushed the framework-cache-directory branch from cdf1046 to 03d3e35 Compare December 16, 2016 18:01
javiereguiluz
javiereguiluz reviewed Dec 23, 2016
View reviewed changes
reference/configuration/framework.rst

**type**: ``string`` **default**: ``cache.app``

A cache adapter.
Copy link
Member

Choose a reason for hiding this comment

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

This description should be improved to explain the purpose of this option. Thanks!

@geoffrey-brier geoffrey-brier force-pushed the framework-cache-directory branch 3 times, most recently from 05dcff7 to ff46950 Compare December 28, 2016 17:45
@geoffrey-brier geoffrey-brier force-pushed the framework-cache-directory branch from ff46950 to 9cb31b2 Compare January 26, 2017 13:59
@geoffrey-brier
Copy link
Contributor Author

@javiereguiluz Any idea why the deployment to platform.sh fails ? In any case, the PR is good for another review :)

@javiereguiluz
Copy link
Member

@geoffrey-brier all the builds are failing ... but our friends at Platform.sh have already fixed the issue and provided two possible solutions. We're deciding which one to use.

wouterj
wouterj approved these changes Apr 15, 2017
View reviewed changes
Copy link
Member
@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

Looks good! Left a few minor comments that can be fixed during the merge.

reference/configuration/framework.rst
.. note::

The framework bundle ships with multiple adapters: apcu, doctrine, system,
filesystem, psr6, and redis.
Copy link
Member

Choose a reason for hiding this comment

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

This note content should be added to the setting description imo

reference/configuration/framework.rst
default_psr6_provider
.....................

**type**: ``string`` **default**: ``%kernel.cache_dir%/pools``
Copy link
Member

Choose a reason for hiding this comment

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

default should be removed

reference/configuration/framework.rst

**type**: ``string``

The service name to use as your default Doctrine provider.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add the service name of this "default provider": "This is available as cache.doctrine service."

reference/configuration/framework.rst

**type**: ``string`` **default**: ``%kernel.cache_dir%/pools``

The service name to use as your default PSR-6 provider.
Copy link
Member

Choose a reason for hiding this comment

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

Same here: "This is available as cache.psr6 service"

reference/configuration/framework.rst

**type**: ``string`` **default**: ``redis://localhost``

The dsn to use by the Redis provider.
Copy link
Member

Choose a reason for hiding this comment

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

...and here: "This is available as cache.redis service"

reference/configuration/framework.rst
Your pool name must differ from ``cache.app`` or ``cache.system``.

adapter
#######
Copy link
Member

Choose a reason for hiding this comment

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

These should be " as well

xabbuh added a commit that referenced this pull request Apr 15, 2017
@xabbuh
feature #7245 [Cache] Document "framework.cache" nodes (geoffrey-brie…
c5bb078 
…r, javiereguiluz)

This PR was submitted for the 3.1 branch but it was merged into the 3.2 branch instead (closes #7245).

Discussion
----------

[Cache] Document "framework.cache" nodes

Commits
-------

c4f0e38 Improve doc
9b0e46e Minor fixes
12e9a3c [Cache] Document "framework.cache" node
xabbuh added a commit that referenced this pull request Apr 15, 2017
@xabbuh
[#7245] apply review comments
af876eb
@xabbuh
Copy link
Member
xabbuh commented Apr 15, 2017

Thank you very much for this great PR @geoffrey-brier and congratulations for your first contribution to the Symfony documentation. I have merged your PR into the 3.2 branch (as Symfony 3.1 is not maintained anymore) and applied @wouterj's comments in af876eb.

@xabbuh xabbuh closed this Apr 15, 2017
@geoffrey-brier geoffrey-brier deleted the framework-cache-directory branch April 16, 2017 08:39
@geoffrey-brier
Copy link
Contributor Author

@xabbuh Thank you, very glad I could help ;)

@HeahDude HeahDude mentioned this pull request Apr 19, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javiereguiluz javiereguiluz javiereguiluz left review comments

@xabbuh xabbuh xabbuh left review comments

@Jean85 Jean85 Jean85 left review comments

@wouterj wouterj wouterj approved these changes

Assignees
No one assigned
Labels
Status: Reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@geoffrey-brier @Jean85 @stof @javiereguiluz @xabbuh @wouterj @carsonbot
0