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

Composer: fix replace directive #113

Closed
wants to merge 1 commit into from
Closed

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Dec 2, 2023

Description

Follow-up on #1

I have a feeling that the replace directive I put in is not correct.

The information about it in the Composer documentation is a bit sparse, but let's see if this works better.

Ref: https://getcomposer.org/doc/04-schema.md#replace

Suggested changelog entry

N/A

composer.json Outdated
@@ -44,7 +44,7 @@
"phpunit/phpunit": "^4.0 || ^5.0 || ^6.0 || ^7.0"
},
"replace": {
"squizlabs/php_codesniffer": "> 2.0"
"squizlabs/php_codesniffer": "self.version"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd pin the last known release before the fork here.

Suggested change
"squizlabs/php_codesniffer": "self.version"
"squizlabs/php_codesniffer": "2.7.2"

This would basically tell Composer: This package is a drop-in replacement for the last release of the old project before we've forked it. If Squiz decided to release a version 2.8.0 from the old repository one fine day (you never know), it might ship with features or API changes that our 2.8.0 doesn't have.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@derrabus Well, I tagged all releases since the 2.0.0 release to make it easier for people to switch over, if for no other reason than that the files in vendor/bin need to be replaced and what with various external standards/packages having different PHPCS requirements, I didn't want this package only having "new" tags being a blocker for people to make the switch.

That's why I originally set it to > 2.0, but I'm finding that doesn't work the way I'd hoped.

So, if I understand what you are saying correctly, I should set it to 2.0.0 instead ?

Note: the "old" tags have the original code, so I've not changed the package name or added the replace directive in those, I'm not rewriting history.

Copy link

@derrabus derrabus Dec 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, you're not rewriting history, so none of the old releases has this replace directive. This is fine, but it also means that the old releases in this repository don't replace anything.

Note that the replace directive is not applied to all releases in this repository, but only for those releases where it appears in the composer.json file. So, if you add that directive to composer.json and then tag a release, the directive is applied to that very release and every subsequent release until you remove the directive again.

Sorry, my previous statement was confusing: replace every 2 in my statement with 3. I wasn't fully awake this morning, apparently. 🙈

Let's start over.

Suggested change
"squizlabs/php_codesniffer": "self.version"
"squizlabs/php_codesniffer": "3.7.2"

My assumption is:

  • The last Squiz release is 3.7.2.
  • This repository was forked 3.7.2, but contains no breaking changes for now.
  • Development continues on this repository.
  • We are going to release 3.8.0 from this release only.

Is that correct? In that case, my suggestion is to tell composer that your PHPCS replaces Squiz' PHPCS 3.7.2 for as long as you remain compatible.

  • Our 3.8.0 replaces Squiz 3.7.2.
  • Our 3.8.1 replaces Squiz 3.7.2.
  • Our 3.9.0 replaces Squiz 3.7.2.
  • Our 4.0.0 replaces nothing and conflicts with "squizlabs/php_codesniffer": "*".

So, if in my app, I'm installing phpcsstandards/php_codesniffer in version 3.8.0 along with a package that requires "squizlabs/php_codesniffer": "^3.5", Composer will accept that because our PHPCS acts as a replacement for "squizlabs/php_codesniffer": "3.7.2" and thus fulfills the requirement.

The problem with self.version is that we would claim that our PHPCS always replaces Squiz' PHPCS with the same version, e.g. our 3.8.0 replaces Squiz' 3.8.0. This is dangerous because we don't own the Squiz repository and if Squiz were to release their own 3.8.0, it will surely not be compatible with ours. We simply cannot make that claim.

Does that make sense to you?

I understand that this package replacing stuff can be very confusing. If I can be on help, I can also offer you a quick video call where I try to answer all the question that you might have.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be great as I admit, this one is confusing the heck out of me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@derrabus I've send you a DM on Mastadon to set a call up.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just curious, assuming we tag a 3.7.3 of this repository, that is a replacement of 3.7.2 from Squiz with just the composer replace change, no new features or BC break, wouldn't the new 3.7.3 be a replacement for all ^3.0,<=3.7.2 from Squiz?

@jrfnl jrfnl force-pushed the feature/composer-fix-replace branch from 9a0714e to b73cd04 Compare December 2, 2023 20:19
Copy link

@sanmai sanmai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach makes total sense! (I used replace on one of the forks.)

@Xerkus
Copy link

Xerkus commented Dec 4, 2023

@jrfnl at Laminas when migrating from Zend Framework we also migrated prior releases.

To accommodate that migrated tags were rewritten in new repository. We had to change PHP namespace (with aliasing bridge) and package name but in your case it would need to be only package name.
All tags must have replace directive "squizlabs/php_codesniffer": "self.version" to be a drop-in replacement.

If you sure no new tags would be issued on original repository then fixed version works for the branch and new releases otherwise I would probably go for a range "squizlabs/php_codesniffer": "~3.7.2".

Change in this PR is good.

Since you retain namespace new major releases would need to declare conflict with all versions of original package instead. I see that handled too.

I have a feeling that the `replace` directive I put in is not correct.

The information about it in the Composer documentation is a bit sparse, but let's see if this works better.

Ref: https://getcomposer.org/doc/04-schema.md#replace
@jrfnl jrfnl force-pushed the feature/composer-fix-replace branch from b73cd04 to 733827d Compare December 6, 2023 01:16
@jrfnl
Copy link
Member Author

jrfnl commented Dec 6, 2023

Looks like this may not be needed after all - #135

@jrfnl jrfnl removed this from the 3.8.0 milestone Dec 6, 2023
@jrfnl jrfnl closed this in #135 Dec 6, 2023
@jrfnl jrfnl deleted the feature/composer-fix-replace branch December 6, 2023 23:28
@jrfnl
Copy link
Member Author

jrfnl commented Dec 6, 2023

Thank you everyone for pitching in, the package rename has been reverted. The package will continue under its original name, so this is no longer relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants