Adding a public method on a non-final class is to be considered a BC break

[Ocramius]

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.

[Majkl578]

It's technically correct, but IMO way too pedantic. 🤔

[Ocramius]

Still gonna add it to the default set with a new major: I'll tackle #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 👍

[Ocramius]

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 #52 before implementing this.

[cebe]

It's technically correct, but IMO way too pedantic. thinking

In case you need a real world example that crashed a lot of things: yiisoft/yii2#14441 (comment) ;)

[Majkl578]

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).

[goetas]

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)

[Ocramius]

@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)

[goetas]

(see #52)

great

[Ocramius]

Moving out of 3.0.0: won't get to it for now.

[Ocramius]

Not going to be able to drag this into 6.0 - removing milestone for now.