Skip to content

Fix hash_file options on PHP before 8.5 - #757

Closed
lcmialichi wants to merge 1 commit into
thecodingmachine:masterfrom
lcmialichi:copilot/fix-hash-file-php81
Closed

lcmialichi wants to merge 1 commit into
thecodingmachine:masterfrom
lcmialichi:copilot/fix-hash-file-php81

Conversation

@lcmialichi

Copy link
Copy Markdown

Summary

Safe\hash_file() always forwards its optional $options argument to native hash_file(). The native fourth argument is only available starting in PHP 8.5, so on PHP 8.1-8.4 the wrapper throws ArgumentCountError even when callers omit $options (the default [] is still forwarded).

This changes the wrapper to pass $options only on PHP 8.5+, while preserving the existing false-result handling and HashException behavior. It also adds a regression test for the default-argument call.

Verification

  • PHP 8.1: default Safe\hash_file('sha256', $file) matches native output.
  • PHP 8.5: default call and MurmurHash seed options match native output.
  • PHP syntax checks pass on PHP 8.1 and 8.5.
  • The full PHPUnit suite could not be run in this environment because the disposable container cannot validate the Packagist TLS certificate; TLS verification was not disabled.

@lcmialichi lcmialichi left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Correction after validation: PHP.Watch documents the fourth hash_file() $options parameter as added in PHP 8.1. The official PHP 8.1.34 image exposes four parameters and accepts the call; PIE 1.5.1 also installed and loaded the extension end-to-end under PHP 8.1.34. The earlier failure was limited to Ubuntu's 8.1.2-1ubuntu2.26 build, which reports only three parameters. This PR's PHP_VERSION_ID < 8.5 branch would disable documented options on PHP 8.1–8.4 and is not the right fix. I recommend closing this PR; if the Ubuntu-specific mismatch needs support, investigate feature detection separately.

@lcmialichi lcmialichi closed this Oct 2, 2026
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.

1 participant