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.

Sign up for GitHub

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

Jump to bottom

fix: remove Google\CRC32 #6532

Merged
merged 4 commits into from
Aug 11, 2023
Merged

fix: remove Google\CRC32 #6532

merged 4 commits into from
Aug 11, 2023

Conversation

vishwarajanand
Copy link
Contributor

@vishwarajanand vishwarajanand commented Aug 4, 2023 •
edited
Loading

Few reasons for removing the Google\CRC32:

  1. crc32c is now supported in PHP7.4+, ref.
  2. google/crc32 was proving to be error prone, see Implicit conversion from float 3988292384 to int loses precision Error in PHP 8.1 #6115.
  3. The dependency has been added with a deprecation flag: Updated php-crc32 for the latest PHP and migrate to Github Actions testing google/php-crc32#16
  4. The new hash function comes from core PHP implementation and outputs the same hash'es, test gist scripts.

Key Changes

  1. Removed Google\CRC32 imports
  2. Removed 3p references fromsupportsBuiltinCrc32c protected function in Storage's Rest connection class.
  3. Fixed UTs and System tests.

@vishwarajanand vishwarajanand marked this pull request as ready for review August 10, 2023 14:02
@vishwarajanand vishwarajanand requested review from a team as code owners August 10, 2023 14:03
noahdietz
noahdietz approved these changes Aug 10, 2023
View reviewed changes
Copy link
Contributor

@noahdietz noahdietz left a comment

Choose a reason for hiding this comment

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

very nice @vishwarajanand thanks

@vishwarajanand vishwarajanand merged commit 9961fb1 into googleapis:main Aug 11, 2023
@release-please release-please bot mentioned this pull request Aug 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bshaffer bshaffer bshaffer left review comments

@noahdietz noahdietz noahdietz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vishwarajanand @bshaffer @noahdietz