From a BC POV, this means that Request::isMethodSafe() was effectively renamed to Request::isMethodCacheable().

Before #19321 + #20205:

    public function isMethodSafe()
    {
        return in_array($this->getMethod(), array('GET', 'HEAD'));
    }

After #19321 + #20205:

    public function isMethodSafe()
    {
        return in_array($this->getMethod(), array('GET', 'HEAD', 'OPTIONS', 'TRACE'));
    }

    public function isMethodCacheable()
    {
        return in_array($this->getMethod(), array('GET', 'HEAD'));
    }

I'm +100 for following RFCs closely. It means shared understanding/expectations. It's super valuable. But it's not okay to do this after it's worked this way the entire Symfony 2.x cycle, and then change this just in some patch release, not even a minor.

I'd have only made this change in a major version (Symfony 3.x). But a new minor version would be acceptable too (2.9.x). In a patch release, that simply blows my mind.

Everybody makes mistakes. Drupal 8 has had accidental BC breaks too. But we fixed them after they were reported.

I think this is nothing more and nothing less than an accidental BC break.

@wimleers thanks for reporting this issue and for the thorough explanation.

I'm asking you to please wait a bit before our maintainers review this issue. I'm afraid it's one for those tricky cases when it's hard to tell if this is a bug fix or a BC break (or both!). Thanks for your patience.

In my opinion, using isMethodSafe() to determine whether or not a request can be cached is actually a bug in Drupal. The fact that a HTTP method is safe simply means that the request will not trigger any modifications.

By the way, why didn't this break in Drupal before? I mean as far as I understand your problem, a HEAD request would have been led to a cache entry that doesn't contain the response body, wouldn't it?

In my opinion, using isMethodSafe() to determine whether or not a request can be cached is actually a bug in Drupal. The fact that a HTTP method is safe simply means that the request will not trigger any modifications.

@xabbuh 👍

That's why we also fixed that in the Symfony HttpCache within #20205

@xabbuh Drupal's cache layer always stores the full response internally and handles HEAD requests when serving, so supporting both GET and HEAD is fine for us.

The issue here is not whether isMethodSafe() is correct for caching or not, it's whether the change should have been made in a patch release.

HttpCache having to be fixed due to the patch release change shows exactly why it should have been held until a minor.

I do not agree with that. Patch releases are exactly for these kind of things (i.e. fixing bugs). And a method that addresses a part of the RFC, but doesn't do it properly is a bug. Thus, it's behaviour was fixed do that the code actually reflects what the method claimed to do. I understand that this broke Drupal and I feel sorry for you about the inconvenience this is causing for you. However, when fixing bugs, we can only assume that people use our interfaces in a way that is conformant with what the API docs tell them. If you start relying on internal implementations instead of the expected outcome you are always risking to break your own projects when we fix bugs.

And the fact that we misused our own API does not mean that the fix was wrong. It just shows that we made the same mistake that Drupal did.

My 2cts here: #20602

This is the Symfony equivalent of the fiasco that is php/php-src@a0724d3

I hope to see this fixed properly.

@teohhanhui Not sure what you are talking about. But this has been fixed nearly three months ago.

The fix was one of "let's maintain BC instead of fixing the bug". There should have been no deprecation because it's a bug fix, but instead the decision was to "deprecate" the buggy behavior and instead forcing everyone who relied on the non-buggy behaviour to pass a false value as parameter.

@xabbuh I refer to your own comments above:
#20562 (comment)
#20562 (comment)

and your objections in #20603 which I fully agree with.

Understood, but the final decision was that BC is more important here. So there will be no further changes in 3.x and the wing behaviour will be dropped in 4.0.