The changes in @internal classes shouldn't be ignored completely

[morozov]

An @internal class still may be used by the consuming code as one of its non-internal parent types. If a breaking change happens in the non-internal subset of the ancestry tree (e.g. the class loses one of its parents), it will affect the consumers of the class even if it's internal.

See doctrine/dbal#4100 (comment).

[Ocramius]

Would need a reproducer: AFAIK, we already skip internal class analysis completely:

BackwardCompatibilityCheck/bin/roave-backward-compatibility-check.php

Line 84 in 15611fe

new ClassBased\SkipClassBasedErrors(new ClassBased\ExcludeAnonymousClasses(new ClassBased\ExcludeInternalClass(

[morozov]

This is the gist of it. I'll see if I can put together a test.

// library
class Parent1 extends Exception
{
}

class Parent2 extends Exception
{
}

/**
 * @internal
 */
class Internal extends Parent1
{
}

function x() {
    throw new Internal();
}

// consumer
try {
    x();
} catch (Parent1 $e) {
}

Breaking changes:

- class Internal extends Parent1
+ class Internal extends Parent2

[Ocramius]

Hmm, I think it's a sufficient scenario: needs to be tried in integration first, to see whether the bug is in a specific change detector, or in the default configuration.

[morozov]

Hmm, I think it's a sufficient scenario […]

Okay, then I'll leave it up to you.

[stof]

note that changing the parent from Parent1 to Parent2 would not be a BC break if Parent1 was internal (as the consumer should not rely on the Parent1 type)

[Ocramius]

@stof I am not sure about that one: the API inherited by parents is still observable downstream. I'm also not sure why you'd inherit from an @internal symbol, but that's probably secondary.

That would probably need to be tested separately.

[Ocramius]

I tried this locally with a repo like following:

commit a8aaf1f42c4157e76c29975ee31c0001ca8c5efa
Author: Marco Pivetta <[email protected]>
Date:   Tue Jun 30 23:26:21 2020 +0200

    Initial state

diff --git a/composer.json b/composer.json
new file mode 100644
index 0000000..74f79fe
--- /dev/null
+++ b/composer.json
@@ -0,0 +1,12 @@
+{
+    "name": "ocramius/example-apicompare",
+    "authors": [
+        {
+            "name": "Marco Pivetta",
+            "email": "[email protected]"
+        }
+    ],
+    "autoload": {
+        "classmap": ["src"]
+    }
+}
diff --git a/src/source.php b/src/source.php
new file mode 100644
index 0000000..4e1fd7a
--- /dev/null
+++ b/src/source.php
@@ -0,0 +1,16 @@
+<?php
+
+class Parent1
+{
+}
+
+class Parent2
+{
+}
+
+/**
+ * @internal
+ */
+class Internal extends Parent1
+{
+}

commit b0c35e689841c509a6a3a69a71ee86d4446c7e2a
Author: Marco Pivetta <[email protected]>
Date:   Wed Jul 1 01:05:20 2020 +0200

    Breaking change

diff --git a/src/source.php b/src/source.php
index 4e1fd7a..4d928ca 100644
--- a/src/source.php
+++ b/src/source.php
@@ -11,6 +11,6 @@ class Parent2
 /**
  * @internal
  */
-class Internal extends Parent1
+class Internal extends Parent2
 {
 }

The tool did not report breaking changes, so I probably need a more precise reproducer

[morozov]

The tool did not report breaking changes, so I probably need a more precise reproducer

Well, this is the bug. It is a breaking change that should have been reported.

[Ocramius]

Hmm, no, if the class is internal, it isn't covered by BC at all

[morozov]

It's not about the class itself, it's about x() that starts throwing a subtype of Parent2 instead of a subtype of Parent1.

[Ocramius]

Aaaaah, I totally misunderstood the issue then.

Will try again, bit not today at this point 😬

[Ocramius]

Hmm, we don't have any BC check on @throws at all.

Few things come to mind:

1. an @internal class should never be part of a catch in consumer code
2. the documented @throws type should likely be the public type defined by the library

Overall, the source of the BC break is not the internal class (which cannot be referenced in any way: consider it "private"), but rather the @throws code location, which should document that either Parent1 or Parent2 are thrown.

Right now, this library has no handling for @throws at all

[stof]

@Ocramius the BC break is the fact that the internal class (the internal implementation of the exception being thrown) is not catched anymore by consuming code which is catching the public type Parent1 (because the internal exception class lost one of its public ancestor types). This re-parenting of an internal exception class caused a BC break even when the consuming code never knows about that class but only about public types. See the reproducing case: there is no reference to the internal type in the consumer code, only in the library code.

[Ocramius]

Yep, I think this is to be checked by an @throws BC checker though: not by a generic inheritance checker