Merge release 6.1.1 into 6.2.x #432
Merged
+35
−3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…nternal symbols As discovered in #429 by @weirdan, the `Stringable` interface is not added to symbols, when the interface is declared by a `vendor/` package. This leads to the `AncestorRemoved` BC checker (correctly) kicking in, and reporting BC breaks: > Live example: https://github.com/vimeo/psalm/runs/5059860222?check_suite_focus=true > ### Steps to reproduce: > > 1. Add `roave/backward-compatibility-check:6.1` to a project that didn't use `symfony/polyfill-php80` and that also has some classes with `__toString()` method. > > 2. Commit `composer.json` (and `composer.lock` if it's versioned), > > 3. Run `vendor/bin/roave-backward-compatibility-check --from="HEAD^"`. > > > ### Expected > > No backward compatibility breaks should be reported as no project files have changed. > ### Actual > > ``` > .... > [BC] REMOVED: These ancestors of Psalm\Storage\Assertion\InArray have been removed: ["Stringable"] > [BC] REMOVED: These ancestors of Psalm\Storage\FunctionLikeStorage have been removed: ["Stringable"] > [BC] REMOVED: These ancestors of Psalm\Storage\Assertion have been removed: ["Stringable"] > [BC] REMOVED: These ancestors of Psalm\Node\VirtualIdentifier have been removed: ["Stringable"] > [BC] REMOVED: These ancestors of Psalm\Node\VirtualName have been removed: ["Stringable"] > [BC] REMOVED: These ancestors of Psalm\Node\VirtualVarLikeIdentifier have been removed: ["Stringable"] > [BC] REMOVED: These ancestors of Psalm\Node\Name\VirtualRelative have been removed: ["Stringable"] > [BC] REMOVED: These ancestors of Psalm\Node\Name\VirtualFullyQualified have been removed: ["Stringable"] > 125 backwards-incompatible changes detected > ``` > > I also confirmed that it's triggered by `symfony/polyfill-php80` by installing BCC into a separate environment and running `vendor/bin/roave-backward-compatibility-check --from="HEAD^"` with the HEAD commit having just this: > > ```diff > commit 4cc0571866e8405658117613a8106f880116decb (HEAD -> add-bcc-2) > Author: Bruce Weirdan <[email protected]> > Date: Fri Feb 4 03:39:44 2022 +0200 > > Add polyfill > > diff --git a/composer.json b/composer.json > index f06872480..567e5d275 100644 > --- a/composer.json > +++ b/composer.json > @@ -35,7 +35,8 @@ > "openlss/lib-array2xml": "^1.0", > "sebastian/diff": "^4.0", > "symfony/console": "^3.4.17 || ^4.1.6 || ^5.0 || ^6.0", > - "symfony/filesystem": "^5.4 || ^6.0" > + "symfony/filesystem": "^5.4 || ^6.0", > + "symfony/polyfill-php80": "^1.24" > }, > "provide": { > "psalm/psalm": "self.version" > ``` As discovered by @Ocramius ( #429 (comment) ) the problem is that `symfony/polyfill-php80` declares a non-internal `Stringable` interface, and `BetterReflection` rightfully does **NOT** use it when deciding whether to add `Stringable` to a class implementing `__toString()`, because only an internal PHP class would be a valid candidate. > Aha, found it. In `roave/better-reflection`, we only add the `Stringable` interface to the ancestors in a `ReflectionClass` of our sources if it is an internal class ( https://github.com/Roave/BetterReflection/blob/d5c9a1afe2ed031c1c86aae58d22f70edb586c67/src/Reflection/ReflectionClass.php#L1113-L1144 ) > > ``` > $stringableInterfaceReflection = $this->reflectClassForNamedNode(new Node\Name($stringableClassName)); > > if ($stringableInterfaceReflection->isInternal()) { > $interfaces[$stringableClassName] = $stringableInterfaceReflection; > } > ``` > > Obviously, the `symfony/polyfill-php80` is not an internal class, and therefore it is skipped. The trouble is that `Reflector#reflectClass()` finds the one under `symfony/polyfill-php80` before looking at the internal source locators. > > This could potentially be fixed by sorting the source locators differently (currently trying that). > > NOTE: it is **OK** to ignore `Stringable` if it didn't come from an **internal** source locator - that's because nobody knows what nightmarish contraption people may have invented before PHP 7.x. With this commit, we put the example in a reproducible test scenario
…talled.json` dependencies In PHP, internal symbols always take priority, since the engine already declared: by moving the `PhpInternalSourceLocator` higher up in the list of aggregated source locators, we prevent internal PHP symbols from being shadowed by their corresponding polyfills (if any are declared). This simple change, for example, prevents `symfony/polyfill-php80` from declaring `Stringable` in the context of reflection, when `Stringable` is already declared by the engine. Fixes #429
…the `AggregateSourceLocator` produced by `LocateDependenciesViaComposer` These assertions were quite fragile to start with.
…cator-considered-before-vendor-polyfills #429 polyfills should not override reflected internal PHP symbols
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Release Notes for 6.1.1
6.1.x bugfix release (patch)
6.1.1
bug