-
Notifications
You must be signed in to change notification settings - Fork 61
Adding support for user context for symfony HttpCache #145
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
Conversation
src/SymfonyCache/HttpCache.php
Outdated
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 keep this kernel very minimal. The component kernel does not define the getOptions method anyways. And i guess people using the component directly are used to hand-craft their things.
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 would call it EventDispatchingHttpCache rather than just HttpCache, to make the name explicit about the difference to the parent class
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.
should we do the same in the bundle?
9737d7f to
164fa5d
Compare
164fa5d to
131849d
Compare
|
okay, tests are green again. last round of feedback @lolautruche @ddeboer ? |
131849d to
4711ba6
Compare
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.
Wrong order.
Should be the following (opposite of array_merge())
$this->options = $options + array(
'default' => 'value'
)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.
ouch, good catch
bf07d2d to
75ce5f4
Compare
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.
this is the wrong order too
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.
btw, why merging the default yourselves ? you should test passing incomplete options
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 order here is correct. i want some test headers.
the reason to merge is so that i can do the right headers in the tests below. i guess i have to split the two.
75ce5f4 to
8cf14a4
Compare
|
Looks great to me. However, docs are still missing. @dbu Can you add doc changes to this PR? We probably need:
|
7a18787 to
d28f1a9
Compare
|
alright, i just added a bunch of documentation. |
d28f1a9 to
6e14f3b
Compare
|
fixed the doc to not have a duplicated reference label. fine like this? anything we need to do or can we merge? |
README.md
Outdated
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.
‘component’ is a bit misleading, as HttpCache is no Symfony Component. Better: ‘Symfony Reverse Proxy’.
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.
hm. "Symfony Reverse Proxy from the http-kernel component"? i want to point out that its about a component ,and not the symfony framework.
|
Thanks @dbu and @lolautruche. Added comments about some minor details. Let’s fix them and merge this! 😄 |
…http-kernel component
6e14f3b to
ac50037
Compare
|
thanks, updated |
Adding support for user context for symfony HttpCache
Moving most of FriendsOfSymfony/FOSHttpCacheBundle#177 to the library.