Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding a parameter to an *internal* constructor should not be considered a break. #723

Closed
weirdan opened this issue Dec 21, 2022 · 5 comments
Assignees
Labels
Milestone

Comments

@weirdan
Copy link
Contributor

weirdan commented Dec 21, 2022

Change: vimeo/psalm@b2ccf24 (#8961)

BCC reports the following issue (the number of required arguments changed):

image

But it's:

  1. a constructor (discussed in Are changes to the constructor really BC breaks? #497)
  2. an internal method
  3. of a final class

So this looks like a false-positive to me.

@Ocramius Ocramius added the bug label Dec 21, 2022
@Ocramius Ocramius added this to the 7.4.1 milestone Dec 21, 2022
@weirdan
Copy link
Contributor Author

weirdan commented Dec 21, 2022

This was reported with BCC 6.1, but the issue remains in 8.2 as well.

@Ocramius
Copy link
Member

I'm confused by my own milestones 🤣

Taking a look shortly BTW: I think this may be a set-up problem in bin/

@Ocramius
Copy link
Member

Ocramius commented Dec 21, 2022

So, I tried it on vimeo/psalm with the patch you were referring to:

../../roave/BackwardCompatibilityCheck/bin/roave-backward-compatibility-check --from=origin/master --to=origin/pr/8961
<SNIP>
[BC] CHANGED: The number of required arguments for Psalm\Plugin\EventHandler\Event\StringInterpreterEvent#__construct() increased from 1 to 2
[BC] CHANGED: The number of required arguments for Psalm\Plugin\EventHandler\Event\BeforeAddIssueEvent#__construct() increased from 2 to 3
2 backwards-incompatible changes detected

I applied a config change:

diff --git a/bin/roave-backward-compatibility-check.php b/bin/roave-backward-compatibility-check.php
index 2edcf53..c261d6a 100644
--- a/bin/roave-backward-compatibility-check.php
+++ b/bin/roave-backward-compatibility-check.php
@@ -208,7 +208,7 @@ use function file_exists;
                                 )),
                             )),
                             new ClassBased\SkipClassBasedErrors(new ClassBased\MethodChanged(
-                                new MethodBased\SkipMethodBasedErrors(new MethodBased\OnlyPublicMethodChanged(
+                                new MethodBased\SkipMethodBasedErrors(new MethodBased\OnlyPublicMethodChanged(new MethodBased\ExcludeInternalMethod(
                                     new MethodBased\MultipleChecksOnAMethod(
                                         new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodBecameFinal()),
                                         new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodConcretenessChanged()),
@@ -226,7 +226,7 @@ use function file_exists;
                                             ),
                                         )),
                                     ),
-                                )),
+                                ))),
                             )),
                         ),
                     )),
../../roave/BackwardCompatibilityCheck/bin/roave-backward-compatibility-check --from=origin/master --to=origin/pr/8961
<SNIP>
No backwards-incompatible changes detected

Will commit change shortly :)

Ocramius added a commit that referenced this issue Dec 21, 2022
Because the configuration for final and non-final classes is almost mirrored, this
is a code duplication problem: the config is large and complex, and these things
kinda happen.

Fixes #723.

Ref: #723 (comment)

Quoting:

> So, I tried it on `vimeo/psalm` with the patch you were referring to:
>
> ```shell
> ../../roave/BackwardCompatibilityCheck/bin/roave-backward-compatibility-check --from=origin/master --to=origin/pr/8961
> <SNIP>
> [BC] CHANGED: The number of required arguments for Psalm\Plugin\EventHandler\Event\StringInterpreterEvent#__construct() increased from 1 to 2
> [BC] CHANGED: The number of required arguments for Psalm\Plugin\EventHandler\Event\BeforeAddIssueEvent#__construct() increased from 2 to 3
> 2 backwards-incompatible changes detected
> ```
>
> I applied a config change:
>
> ```diff
> diff --git a/bin/roave-backward-compatibility-check.php b/bin/roave-backward-compatibility-check.php
> index 2edcf53..c261d6a 100644
> --- a/bin/roave-backward-compatibility-check.php
> +++ b/bin/roave-backward-compatibility-check.php
> @@ -208,7 +208,7 @@ use function file_exists;
>                                  )),
>                              )),
>                              new ClassBased\SkipClassBasedErrors(new ClassBased\MethodChanged(
> -                                new MethodBased\SkipMethodBasedErrors(new MethodBased\OnlyPublicMethodChanged(
> +                                new MethodBased\SkipMethodBasedErrors(new MethodBased\OnlyPublicMethodChanged(new MethodBased\ExcludeInternalMethod(
>                                      new MethodBased\MultipleChecksOnAMethod(
>                                          new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodBecameFinal()),
>                                          new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodConcretenessChanged()),
> @@ -226,7 +226,7 @@ use function file_exists;
>                                              ),
>                                          )),
>                                      ),
> -                                )),
> +                                ))),
>                              )),
>                          ),
>                      )),
> ```
>
> ```shell
> ../../roave/BackwardCompatibilityCheck/bin/roave-backward-compatibility-check --from=origin/master --to=origin/pr/8961
> <SNIP>
> No backwards-incompatible changes detected
> ```
Ocramius added a commit that referenced this issue Dec 21, 2022
…-issues-also-on-final-classes

Fix #723 by ignoring BC issues on `@internal` methods of `final` classes
@Ocramius Ocramius self-assigned this Dec 21, 2022
@Ocramius
Copy link
Member

Handled in #724

@Ocramius
Copy link
Member

Released BTW :)

uzibhalepu added a commit to uzibhalepu/BackwardCompatibilityCheck that referenced this issue Aug 6, 2024
Because the configuration for final and non-final classes is almost mirrored, this
is a code duplication problem: the config is large and complex, and these things
kinda happen.

Fixes #723.

Ref: Roave/BackwardCompatibilityCheck#723 (comment)

Quoting:

> So, I tried it on `vimeo/psalm` with the patch you were referring to:
>
> ```shell
> ../../roave/BackwardCompatibilityCheck/bin/roave-backward-compatibility-check --from=origin/master --to=origin/pr/8961
> <SNIP>
> [BC] CHANGED: The number of required arguments for Psalm\Plugin\EventHandler\Event\StringInterpreterEvent#__construct() increased from 1 to 2
> [BC] CHANGED: The number of required arguments for Psalm\Plugin\EventHandler\Event\BeforeAddIssueEvent#__construct() increased from 2 to 3
> 2 backwards-incompatible changes detected
> ```
>
> I applied a config change:
>
> ```diff
> diff --git a/bin/roave-backward-compatibility-check.php b/bin/roave-backward-compatibility-check.php
> index 2edcf53..c261d6a 100644
> --- a/bin/roave-backward-compatibility-check.php
> +++ b/bin/roave-backward-compatibility-check.php
> @@ -208,7 +208,7 @@ use function file_exists;
>                                  )),
>                              )),
>                              new ClassBased\SkipClassBasedErrors(new ClassBased\MethodChanged(
> -                                new MethodBased\SkipMethodBasedErrors(new MethodBased\OnlyPublicMethodChanged(
> +                                new MethodBased\SkipMethodBasedErrors(new MethodBased\OnlyPublicMethodChanged(new MethodBased\ExcludeInternalMethod(
>                                      new MethodBased\MultipleChecksOnAMethod(
>                                          new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodBecameFinal()),
>                                          new MethodBased\SkipMethodBasedErrors(new MethodBased\MethodConcretenessChanged()),
> @@ -226,7 +226,7 @@ use function file_exists;
>                                              ),
>                                          )),
>                                      ),
> -                                )),
> +                                ))),
>                              )),
>                          ),
>                      )),
> ```
>
> ```shell
> ../../roave/BackwardCompatibilityCheck/bin/roave-backward-compatibility-check --from=origin/master --to=origin/pr/8961
> <SNIP>
> No backwards-incompatible changes detected
> ```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants