-
Notifications
You must be signed in to change notification settings - Fork 83
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
Refactor docs #111
Refactor docs #111
Conversation
.. code-block:: bash | ||
|
||
$ composer require sensio/framework-extra-bundle | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should also give the line to instantiate the bundle in the kernel, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
great work, this is coming good! added some comments so we see the open ends in the PR and some inputs. |
Rules | ||
===== | ||
|
||
Configure the bundle under the ``fos_http_cache`` key. The configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the first sentence is a general intro and does not belong here.
@ddeboer how is the state here? i think this is the last blocker for RC1 and i would love to release that this week. anything i can help with here? |
I guess working on the same doc files together at the same time will only complicate things. I’ll push an update tonight, so if it would be most helpful if you could go through that and your criticism. By the way, do you want to skip beta and go directly to RC? |
ups, i thought we have a beta already. hm. lets tag a beta then and announce it, as soon as this PR is merged. and agreed that editing in the same place is probably tricky. ping me when you pushed your update. and then lets try to wrap it up and maybe add some follow-up tickets for things that could be improved, so we can work separately on them. |
Sounds like a plan! |
Pushing some updates. A built version is available at http://foshttpcachebundle.readthedocs.org/en/refactor-docs/. |
@dbu Not sure where this should go (removed it from the docs for now). Can you place it where it belongs?
Also, can you do the Flash Message config chapter? |
Still to do:
|
great. sure i can handle the flash message thingy. i will also look into some sort of common header for each feature chapter to say whether it needs the cache manager and varnish configuration or something like that. with the split between library and bundle, this is a bit tricky to see. |
You can also use expressions_ in the route parameter values: | ||
|
||
.. code-block:: php | ||
You can also use expressions_ in the route parameter values:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is the expression in the next example? it looks very similar to the previous one to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does indeed. The difference is that latest
does not come from the request attributes (it’s hard-coded) and id
does. They look the same because the InvalidationSubscriber tries to evaluate each parameter as an expression against request attributes, and fails silently when it doesn’t seem to be an expression (as in the case of latest
).
Do you have an idea as to how we could make this clearer? Should we make the behaviour more explicit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, so "id" is tried to be the expression looking for the value id
and if that fails, it is the string "id"?
maybe add a note with what you just explained here. its valuable information, also when wanting a static expression that happens to match a parameter name...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. You could force a string, e.g. id, by giving "'id'"
(extra quotes). This is different from how we do tags: @Tag("string")
versus @Tag(expression="id")
. I think I prefer this inconsistency over something like: @InvalidateRoute("route", params={"type" = {"expression": "id"}})
(is this even possible?). Do you agree? I’ll update the docs to make this clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
theoretically you could do params_expression (or param_expressions?). and then even merge the two. if that is no big pain, we might actually want to do that to avoid that WTF moment when a name hits a parameter name. doing it later would be a BC problem...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would the syntax be like, in your proposal? I think just supplying one list of route params, and differentiating between them with a literal string versus key/value {"expression": "id"}
is most user-friendly. Do you have time to open a PR for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
created #117, lets see. i think we should merge now, and then clean that bit up.
if you agree, can you squash and merge?
ftr: i decided to drop this ESI hint. its extremly specific and if anything would belong somewhere into the symfony cookbook, not here. |
|
||
.. toctree:: | ||
|
||
helpers/flash-message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need a separate Helpers chapter? Should more items be listed here, or do we expect any to pop up soon? What about just upgrading the Flash Message helper to a fully-fledged Feature chapter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what i like about this is that it makes this thing less prominent than the rest. it is less important. and we might turn up with other things too. but if you prefer, you can also move this, and we can restructure when/if we have more helpers turning up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no strong feelings about this. The helper just looks a bit lonely in there. 😉
Fix #94. Add bundle activation to installation Improve @tag section in Annotations Include match partial in invalidation chapter Update user context docs Fix typos Add chapter intros Add chapter headers Add Debug config chapter Add flash message chapter (todo) Update proxy client chapter Remove default config chapter Add Configuration chapter header Re-organise configuration chapters Match is now linked to instead of included, and included in the TOC too.
* fix links to FOSHttpCache docs * a little clean-up
Okay, squashed. If Travis is green you may press the big green button. 😄 |
yay, great work! |
Fix #94.