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 <ocramius@gmail.com>
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": "ocramius@gmail.com"
+        }
+    ],
+    "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 <ocramius@gmail.com>
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 😬