BackwardCompatibilityCheck
BackwardCompatibilityCheck copied to clipboard
Roave
Adding a public method on a non-final class is to be considered a BC break
Take following example:
class Counter
{
private $count = 0;
public function increment() : void {
$this->count += 1;
}
public function count() : int {
return $this->count;
}
}
class LoggingCounter extends Counter
{
private $logger;
private $originalCounter;
public function __construct(Logger $logger, Counter $originalCounter)
{
$this->logger = $logger;
$this->originalCounter = $originalCounter;
}
public function increment() : void {
$this->logger->log('increment');
$this->originalCounter->increment();
}
}
If Counter is modified to add an increment2 method, LoggingCounter fails (this is partly described in my article "When to declare Classes Final" ).
class Counter
{
private $count = 0;
public function increment() : void {
$this->count += 1;
}
public function increment2() : void {
$this->count += 2;
}
public function count() : int {
return $this->count;
}
}
Here's a test to show that:
class LoggingCounterTest extends TestCase
{
public function incrementAndIncrement2() : void
{
$counter = new LoggingCounter($this->createMock(Logger::class), new Counter());
$counter->increment();
$counter->increment2();
self::assertSame(3, $counter->count());
}
}
This is because decorators require an update for each parent class public API change. Therefore, we should mark any addition of methods to an open class as a BC break.
Still gonna add it to the default set with a new major: I'll tackle https://github.com/Roave/BackwardCompatibilityCheck/issues/52 as well.
It is true that these are pedantic, but I'm really just trying to categorise the BC breaks here, not saying this will fit for everyone :+1:
Hmm, previous comment got lost due to timeouts.
Wanted to clarify that I'm just categorising and enabling BC breaks as they are discovered/remembered (this one was long known, just forgotten), even if they are extremely strict, so this will appear in the defaults for the next major.
I'll try to tackle https://github.com/Roave/BackwardCompatibilityCheck/issues/52 before implementing this.
It's technically correct, but IMO way too pedantic. thinking
In case you need a real world example that crashed a lot of things: https://github.com/yiisoft/yii2/pull/14441#issuecomment-315127527 ;)
We also have one in Doctrine: EntityRepository::count() with different signature than the one that comes from Countable, broke a lot of stuff in userland for people who implemented Countable (which doesn't make much sense, but anyway).
as @Majkl578 said, adding bc breaks in this case is correct but a bit pedantic. What abound introducing "optional" rules where the user can opt-in? (based on their BC promise)
@goetas the library will provide a mathematical set of what is right/wrong, and release a new major version each time a new rule is added to that set: configuring a subset of BC breaks will be up to consumers (see #52)