Skip to content

Basic documentation of cache plugin methods option #174

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

Merged
merged 4 commits into from
Feb 20, 2017

Conversation

tuupola
Copy link
Contributor

@tuupola tuupola commented Feb 14, 2017

No description provided.

Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

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

thank you. i have some styling input and the thing we discussed on the code change, mention case sensitivity

@@ -36,18 +36,20 @@ control headers from the server as specified in :rfc:`7234`. It needs a
Options
-------

The third parameter to the ``CachePlugin`` constructor takes an array of options. The plugin has 3 options you can
The third parameter to the ``CachePlugin`` constructor takes an array of options. The plugin has four options you can
configure. Their default values and meaning is described by the table below.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe just:

"The available options are:"

+===========================+=====================+======================================================+
| ``default_ttl`` | ``0`` | The default max age of a Response |
+---------------------------+---------------------+------------------------------------------------------+
| ``respect_cache_headers`` | ``true`` | Whatever or not we should care about cache headers |
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Whatever/Whether/

(i see you only reformatted this, but just jumped into my eyes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe "Whether we should care about cache headers or not" would be proper english.

'methods' => ['GET', 'HEAD', 'POST'],
];

You can cache any valid request method.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe:

"methods" overwrites the default methods, if you want to keep caching GET and HEAD requests, you need to repeat them. You can specify any request method that conforms to RFC-7230. Note that request methods are case sensitive, so post and POST are not the same.

Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

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

thanks, looks good to me

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

I like it and I think it is good to merge if the PR gets merged at its current state.

];

The ``methods`` setting overrides the defaults. If you want to keep caching ``GET`` and ``HEAD`` requests, you need
to include them. You can specify any request method that conforms to RFC-7230. Request methods are case sensitive,
Copy link
Member

Choose a reason for hiding this comment

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

"Request methods are case sensitive,". I do not like this implementation. But I made some questions on the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

php-http/cache-plugin#24 was now changed to force upper case. please change this line to say that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, can #24 now be considered feature frozen?

Copy link
Contributor

Choose a reason for hiding this comment

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

i just merged php-http/cache-plugin#24 ;-)

];

The ``methods`` setting overrides the defaults. If you want to keep caching ``GET`` and ``HEAD`` requests, you need
to include them. You can specify any request method that conforms to RFC-7230. Request methods are case sensitive,
Copy link
Contributor

Choose a reason for hiding this comment

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

php-http/cache-plugin#24 was now changed to force upper case. please change this line to say that.

@dbu
Copy link
Contributor

dbu commented Feb 20, 2017

any other input or can we merge like this? should we maybe add some "since" notes?

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Good!

I do not think we need "since". All the docs relate to the latest version.

@Nyholm Nyholm merged commit 627cf43 into php-http:master Feb 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants