Skip to content

Conversation

@blizzz
Copy link
Member

@blizzz blizzz commented Aug 29, 2016

…7230#section-3.3

from owncloud/core#25835

@nickvergessen @MorrisJobke you have had concerns in the original PR

@blizzz blizzz added 3. to review Waiting for reviews downstream labels Aug 29, 2016
@blizzz blizzz added this to the Nextcloud 11.0 milestone Aug 29, 2016
@mention-bot
Copy link

@blizzz, thanks for your PR! By analyzing the annotation information on this pull request, we identified @LukasReschke, @DeepDiver1975 and @MorrisJobke to be potential reviewers

if ($this->getStatus() === Http::STATUS_NO_CONTENT
|| $this->getStatus() === Http::STATUS_NOT_MODIFIED
) {
$response = null;
Copy link
Member

Choose a reason for hiding this comment

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

I'd say we should only take this for master and then throw an exception. Don't do magic.

cc @nickvergessen @MorrisJobke

@rullzer
Copy link
Member

rullzer commented Aug 29, 2016

This fix is the wrong way around.
Because lets say I set the right etag. And then a request is made with the IF-NONE-MATCH header that matches. Then the response doesn't know this.

So 👎

@rullzer
Copy link
Member

rullzer commented Aug 31, 2016

Also it is weird that it only does this for 1 response type

@blizzz
Copy link
Member Author

blizzz commented Aug 31, 2016

Then, let's 🔥 it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants