-
-
Notifications
You must be signed in to change notification settings - Fork 74
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
Support Composers "replaces" config, important for CI in mono repos #57
Conversation
src/PackageVersions/Installer.php
Outdated
$rootVersion = $rootPackage->getPrettyVersion() . '@' . $rootPackage->getSourceReference(); | ||
|
||
foreach ($rootPackage->getReplaces() as $replace) { | ||
yield $replace->getTarget() => $rootVersion; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems wrong to me: the replaced package is not installed, so this is not its version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In most cases replace directive is used in conjunction with self.replace version. Actually I didn't yet seen a usage of this using it differently and it's discouraged to use it differently. I didn't want to hardcode there such magic string, but I will if you think it's better. Should I? Also not sure I can get a reference part for such packages yet, I'm on mobile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with showing a version, but that string should somehow show that this is a replacement. Adding the replacement package name to the version would be sufficient, IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I don't care about actual version, so if we show there literal value, like 'self.replace', I'm fine with this. This PR is only to make CI not crash for packages which were not found. Is that ok with you, or we still need to indicate it is replacement? Like self.replace@replaced-by-root/package
?
Just a note: @
will need to be there, as not having it would break packages like https://github.com/Jean85/pretty-package-versions, even though I don't know what's your BC policy regarding that
Ping @Ocramius |
@Jean85 can you check this? |
@@ -162,6 +162,7 @@ public function testDumpVersionsClass() | |||
'foo/bar' => '1.2.3@abc123', | |||
'baz/tab' => '4.5.6@def456', | |||
'tar/taz' => '7.8.9@ghi789', | |||
'some-replaced/package' => 'self.version@aaabbbcccddd', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Jean85 this is what the string would look like. It is fine by me, just need to make sure it isn't a BC break on your side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, I'm good! 👍
I've added regression tests on my side: Jean85/pretty-package-versions@44e26e7
The only thing is that obviously self.version
is not getting replaced with the correct one... Is that an issue on your side? It's probably easily fixable with #56
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have strong feelings, but my (personal) use-case for this library is to create hashes of versions, so that I can decide when to re-generate artifacts. As long as the actual version changes (bit after @
), I'm good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the replace
feature is used by the symfony/symfony
package, I think it would be helpful. If both this and #56 get merged, I could attempt a PR to solve this.
As per discussion in #57 (comment): 🚢 |
Thanks @Jean85! |
This is needed to fix issues such as deprecated-packages/symplify#589