search

willdurand / Negotiation

Content Negotiation tools for PHP.
https://williamdurand.fr/Negotiation/
MIT License
1.41k stars 62 forks source link

BC break? - Fix for issue #81 - silently skip invalid header values coming in from clients #84

Closed sgehrig closed 8 years ago

sgehrig commented 8 years ago

silently skip invalid header values coming in from a client

instead of throwing exceptions on invalid headers from clients, the new code silently skips these header values and continues to parse the header for a best match

willdurand commented 8 years ago

Great!

willdurand commented 8 years ago

Wondering if this is a BC break to be honest. The "BC break" is actually the bug itself, hence it is a bugfix IMO.. I know the behavior of the lib will change because it will not throw an exception anymore, but that behavior was faulty.

sgehrig commented 8 years ago

You're right. I think it's easy to argue that it's not a BC break because, as you said, the original implementation had some flaws.

sgehrig commented 8 years ago

https://twitter.com/phpalcohol/status/770562475951550464:

phpalcohol: @couac does the change break your API or not (were the exceptions explicitly part of your API declaration)? @lsmith

I think this is a good argument.

willdurand commented 8 years ago

thanks @sgehrig!

stof commented 8 years ago

@sgehrig you should rebase your branch so that the commit diff contains only the relevant changes instead of adding changes from other commits in your patch.

Anyway, I think this one qualifies as a bug fix (rejecting the whole negotiation when the client submits an invalid header value among others does not loo like something to be considered as a feature)

sgehrig commented 8 years ago

@stof: I think the PR contained only changes done to fix this issue. It was based on master from somehow this morning - so I think it was the most recent master branch available. The other PR (#82) was a different way to tackle the problem - but it wasn't complete.

dunglas commented 8 years ago

This is indeed a BC break: https://travis-ci.org/api-platform/core/jobs/156291020

willdurand commented 8 years ago

@dunglas do you agree that the behavior was incorrect?

dunglas commented 8 years ago

Yes, this change will allow me to remove some useless code. However it may impact other people.