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

Fixed v4 constraint #115

Merged
merged 1 commit into from
Nov 23, 2020
Merged

Fixed v4 constraint #115

merged 1 commit into from
Nov 23, 2020

Conversation

GrahamCampbell
Copy link
Contributor

4.0.x-dev is the name of a branch, and not a version constraint. You should use a proper version constraint, otherwise anyone else who does use a one will find composer doesn't know what to do.

@jrfnl
Copy link
Member

jrfnl commented Jun 27, 2020

@GrahamCampbell That's because 4.x isn't a tag, it's a development branch and this was added to allow testing with that branch.
IIRC using 4.x won't work until there is a release. (Can't test at the moment)

@GrahamCampbell
Copy link
Contributor Author

GrahamCampbell commented Jun 27, 2020

No, it will work fine. That's the nice thing about composer. It knows how to install 4.0.x dev versions.

@jrfnl
Copy link
Member

jrfnl commented Jun 27, 2020

@GrahamCampbell Thanks. I'll have a play with it again once I'm behind a proper computer to test it.

Copy link
Member

@Potherca Potherca left a comment

Choose a reason for hiding this comment

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

Does exactly what it says on the tin

@Potherca
Copy link
Member

@jrfnl Should I just merge and deploy it as 0.7.1?

@jrfnl
Copy link
Member

jrfnl commented Jun 30, 2020

Sorry for the delay. I've finally had some time to think of some test scenarios and have a play with it.

Scenario 1 - plain install for the plugin itself:

  • Clone this repo
  • If cloned before, make sure to throw away the composer.lock file and the vendor directory.
  • Run composer install

With both the 0.7.0 release as well as with this PR, PHPCS 3.5.5 is installed as expected and desired ("prefer-stable": true).

Scenario 2 - minimal external project setup for PHPCS 3.x (stable)

Note: PHPCSUtils also already allows for PHPCS 4.x and contains external standards which should trigger the plugin.

{
	"name": "jrfnl/plugin-issue-115",
	"require": {
		"php": ">=5.4",
		"squizlabs/php_codesniffer": "^3.1",
		"phpcsstandards/phpcsutils" : "^1.0",
		"dealerdirect/phpcodesniffer-composer-installer": "^0.7.0"
	},
	"minimum-stability": "dev",
	"prefer-stable": true
}

Run composer install - output as expected & correct:

Package operations: 3 installs, 0 updates, 0 removals
  - Installing squizlabs/php_codesniffer (3.5.5): Loading from cache
  - Installing dealerdirect/phpcodesniffer-composer-installer (v0.7.0): Loading from cache
  - Installing phpcsstandards/phpcsutils (1.0.0-alpha3): Loading from cache
Writing lock file
Generating autoload files
PHP CodeSniffer Config installed_paths set to ../../phpcsstandards/phpcsutils

Scenario 3 - minimal external project setup for PHPCS 4.x (unstable)

{
	"name": "jrfnl/plugin-issue-115",
	"require": {
		"php": ">=5.4",
		"squizlabs/php_codesniffer": "^4.0",
		"phpcsstandards/phpcsutils" : "^1.0",
		"dealerdirect/phpcodesniffer-composer-installer": "^0.7.0"
	},
	"minimum-stability": "dev",
	"prefer-stable": true
}

Run composer install - output as expected & correct:

Package operations: 3 installs, 0 updates, 0 removals
  - Installing squizlabs/php_codesniffer (4.0.x-dev 14e9ce7): Cloning 14e9ce7599 from cache
  - Installing dealerdirect/phpcodesniffer-composer-installer (v0.7.0): Loading from cache
  - Installing phpcsstandards/phpcsutils (1.0.0-alpha3): Loading from cache
Writing lock file
Generating autoload files
PHP CodeSniffer Config installed_paths set to ../../phpcsstandards/phpcsutils

otherwise anyone else who does use a one will find composer doesn't know what to do.

In other words, I have not been able to reproduce the issue this PR is supposed to solve as the test scenario did use "proper constraints", while 0.7.0 refers to the dev branch, and the results were as expected, without any problems reported by Composer about it not knowing what to do.

Tested with Composer 1.1.0.8.

@GrahamCampbell Could you provide a test scenario which actually demonstrates the problem ?

@Potherca I'm fine with merging this either way as it doesn't seem to do any harm either.
Based on the above tests, I don't think a new release straight away is warranted though as things seem to work fine as they are.

@jrfnl
Copy link
Member

jrfnl commented Jul 11, 2020

@GrahamCampbell Just checking in - have you had a chance to come up with a reproduction scenario for this issue ?

@GrahamCampbell
Copy link
Contributor Author

I think you're right that it technically works, but it's not a proper version constraint. Composer just works out that you meant to type what I changed it to in this PR.

@jrfnl
Copy link
Member

jrfnl commented Jul 11, 2020

@GrahamCampbell To be fair, I believe it's the other way around, though I could be wrong.

Composer supports branch-based constraints. So when using the "proper" version restraint, as there is no 4.x version available yet, composer looks for a branch which matches and finds the correct branch.
So the branch constraint currently used saves Composer from the overhead of having to do that as it already points to the branch.

@GrahamCampbell
Copy link
Contributor Author

I still maintain that my changes are the best way to do things, and will actually still work once a 4.x release has been tagged.

@jrfnl
Copy link
Member

jrfnl commented Aug 13, 2020

@GrahamCampbell And once there is a (RC) release of PHPCS 4.x, we will definitely make that change. As things stand, PHPCS 4.x is not officially supported (yet), the 4.0.x-dev allowance has, for now, only been added to make it easier for external PHPCS standards to test against PHPCS 4.x, not as an official statement of PHPCS 4.x being supported by this plugin.

@Potherca Potherca self-assigned this Aug 14, 2020
@Potherca Potherca merged commit ac556a3 into PHPCSStandards:master Nov 23, 2020
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.

3 participants