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

chore(deps): require slevomat/coding-standard v8 and squizlabs/php_codesniffer v3.7 #264

Merged
merged 1 commit into from Jun 23, 2022

Conversation

simPod
Copy link
Contributor

@simPod simPod commented Jun 18, 2022

  • Bump min php to 7.2

ostrolucky
ostrolucky previously approved these changes Jun 18, 2022
@greg0ire
Copy link
Member

greg0ire commented Jun 18, 2022

Drop squizlabs/php_codesniffer as dependency since it's inherited from slevomat/coding-standard

This should be reverted. One should not rely on a dependency being transitively installed when referencing things defined in it, and we do:

<exclude name="Squiz.Functions.FunctionDeclarationArgumentSpacing.SpacingAfterVariadic"/>

I think this needs 10.0.x

Why?

@simPod
Copy link
Contributor Author

simPod commented Jun 18, 2022

@simPod simPod changed the title chore(deps): require slevomat/coding-standard v8 chore(deps): require slevomat/coding-standard v8 and squizlabs/php_codesniffer v3.7 Jun 18, 2022
@greg0ire
Copy link
Member

How is it a BC-break? That sniff is not part of the public API of doctrine/coding-standard, is it?

@simPod
Copy link
Contributor Author

simPod commented Jun 18, 2022

I though of a case when user e.g. excluded that sniff by a name. The sniff is enabled by Doctrine CS. Not sure if it's considered BC break or not or what's actually a public api in the context of CS. 🤷🏿‍♀️ Ur call.

@greg0ire
Copy link
Member

greg0ire commented Jun 18, 2022

by a name

If that name is defined by slevomat/coding-standard, then they should require it beforehand. In practice, we don't seem to be doing so ourselves (if you look for instance at doctrine/dbal), but what I mean is that people cannot really complain to us about a BC-break in this case.

what's actually a public api in the context of CS

I think it this case, the public API is the name Doctrine in <rule ref="Doctrine"/>, and that's it.

@simPod simPod changed the base branch from 9.0.x to 10.0.x June 19, 2022 05:23
@simPod simPod changed the base branch from 10.0.x to 9.0.x June 19, 2022 05:23
@derrabus
Copy link
Member

If I may weigh in on the target version discussion: If this change does not justify a major version bump (10.0.0), it certainly deserves a minor version bump (9.1.0) because I believe this is not just a bugfix.

@greg0ire
Copy link
Member

If I may weigh in on the target version discussion: If this change does not justify a major version bump (10.0.0), it certainly deserves a minor version bump (9.1.0) because I believe this is not just a bugfix.

I think you're correct, so I should a create a 9.1.x branch so that this can be retargeted.

Looking at 9.0.0...9.0.x though, I see a couple PRs we all reviewed (including myself 😅 ) have been targeted at the wrong branch (IMO):

I say we merge this one in 9.0.x as well, then merge 9.0.x up into 10.0.x, and release 10.0.0, and then we try to stick to semver 😄

I can contribute more details to https://www.doctrine-project.org/projects/doctrine-coding-standard/en/8.2/reference/index.html#versioning but I think both PRs above are already covered by that paragraph.

@simPod
Copy link
Contributor Author

simPod commented Jun 22, 2022

With v10 lets wait I have few PRs to add

@ostrolucky
Copy link
Member

Somebody with admin rights has to merge this...
image

@greg0ire greg0ire merged commit 094a121 into doctrine:9.0.x Jun 23, 2022
@greg0ire
Copy link
Member

Thanks @simPod !

@simPod simPod deleted the req-8 branch June 24, 2022 07:30
@greg0ire greg0ire added this to the 9.0.1 milestone Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants