Roave / BackwardCompatibilityCheck

:ab: Tool to compare two revisions of a class API to check for BC breaks
MIT License
568 stars 59 forks source link

Why is this not a breaking change? #750

Open loevgaard opened 8 months ago

loevgaard commented 8 months ago
 class HttpClient
 {
-    public function request(string $method, string $url)
+    public function request(string $method, string $url, array $options = [])
     {

     }

My thoughts are: If I extended the HttpClient before this change, then after the change I would get this error:

Fatal error: Declaration of ConcreteHttpClient::request(string $method, string $url) must be compatible with HttpClient::request(string $method, string $url, array $options = [])
loevgaard commented 8 months ago

Something along the same line goes for this example:

 class DataTransferObject
 {
-    protected array $data = [];
+    public array $data = [];

If I extended DataTransferObject before this change and I'd overridden the $data property I would get this error:

Fatal error: Access level to ActualDataTransferObject::$data must be public (as in class DataTransferObject) in /tmp/preview on line 13
Ocramius commented 8 months ago

Agree on the BC break in the OP:

https://3v4l.org/rXi7F

<?php

class HttpClient
{
    //public function request(string $method, string $url)
    public function request(string $method, string $url, array $options = [])
    {
    }
}

class HttpClientDecorator extends HttpClient
{
    public function request(string $method, string $url) {}
}

Produces:

Fatal error: Declaration of HttpClientDecorator::request(string $method, string $url) must be compatible with HttpClient::request(string $method, string $url, array $options = []) in /in/rXi7F on line 13

Process exited with code 255.

I suggest that we add a test case for it, then identify which class would be responsible for making that test green.

Adding optional arguments to a method of a a non-final class is certainly a BC break.

Same agreement on DataTransferObject: I guess we simply never thought of that scenario :)