Add check for object since getHeader can return bool #3251
Conversation
|
Thank you for the contribution, @hiephm, it looks good. Just one question in the code. Also, looks like travis builds have failures related to this change. Could you please update the unit tests? |
|
Internal ticket created: MAGETWO-49732 |
| if ($cacheControlHeader instanceof \Zend\Http\Header\HeaderInterface) { | ||
| $cacheControl = $cacheControlHeader->getFieldValue(); | ||
| $this->addDebugHeader($result, 'X-Magento-Cache-Control', $cacheControl); | ||
| $this->addDebugHeader($result, 'X-Magento-Cache-Debug', 'MISS', true); |
There was a problem hiding this comment.
If getHeader() returns false, why should the header 'X-Magento-Cache-Control' not be sent?
There was a problem hiding this comment.
Because in that case there is no Cache-Control header is set. If we set any value to X-Magento-Cache-Control, it may confuse developers that value is set from Magento, unless we can put a very clear message to identify that fact. That is my thinking.
There was a problem hiding this comment.
I misspoke, I meant X-Magento-Cache-Debug, because it does not rely on the $cacheControl variable. Won't devs still benefit from seeing whether cache was missed? Agree on X-Magento-Cache-Control header
There was a problem hiding this comment.
I agree. I have changed the code like that. (not sure why the test is failed for PHP 5.5, but it doesn't related to this fix)
sshrewz
added
Issue: Ready for Work
[EngCom] Public Pull Requests - GraphQL
This issue happened to me when I used plugin to return
Redirectresult without settingCache-Controlheader.In this case
getHeaderwill returnfalseand cause: