Skip to content

Conversation

@orklah
Copy link
Collaborator

@orklah orklah commented Nov 12, 2021

This brings consistency to the way we use php versions. Before that, sometimes we passed them under the X.Y format, sometimes as two int params and sometimes as an array of two ints...

It also get rid of those horrors:

if ($codebase->php_major_version < 8 ||
   ($codebase->php_major_version === 8 && $codebase->php_minor_version < 1)
) {

and it fixes a couple of bugs like
if (\PHP_VERSION_ID < 80100 && $codebase->php_major_version >= 8 && $codebase->php_minor_version >= 1) {
that would have been a problem in version 9.0

However, it's probably a BC break for plugin users...

Copy link
Collaborator

@weirdan weirdan left a comment

Choose a reason for hiding this comment

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

BC breaks look avoidable. Conversions from version ID to major/minor need to be fixed, and it would be better to move conversions into its own helper class.

@weirdan weirdan added the release:deprecation The PR will be included in 'Deprecated' section of the release notes label Nov 12, 2021
@orklah
Copy link
Collaborator Author

orklah commented Dec 21, 2021

I did not pursue this as a change in current version because I really hate the change in order to avoid the BC break. This will probably be something only in Psalm 5...

@weirdan weirdan added this to the Psalm 5 milestone Dec 21, 2021
@weirdan
Copy link
Collaborator

weirdan commented Dec 21, 2021

This will probably be something only in Psalm 5...

I added the Psalm 5 milestone we can use for such PRs / issues.

weirdan added a commit to weirdan/psalm that referenced this pull request Jan 2, 2022
@weirdan weirdan added release:removed The PR will be included in 'Removed' section of the release notes and removed release:deprecation The PR will be included in 'Deprecated' section of the release notes labels Jan 2, 2022
@weirdan
Copy link
Collaborator

weirdan commented Jan 2, 2022

@orklah mind giving this a once-over? I rebased it, removed deprecated properties and documented BC breaks (in UPGRADING.md).

@orklah
Copy link
Collaborator Author

orklah commented Jan 2, 2022

I'll take a look tonight

@orklah
Copy link
Collaborator Author

orklah commented Jan 2, 2022

That seems correct. Thanks for the documentation and the rebase <3

@weirdan weirdan merged commit 2f50070 into vimeo:master Jan 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release:removed The PR will be included in 'Removed' section of the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants