Skip to content

Fix ZPP for mhash() #5985

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

Closed
wants to merge 1 commit into from
Closed

Fix ZPP for mhash() #5985

wants to merge 1 commit into from

Conversation

kocsismate
Copy link
Member

Related to #5881

@nikic
Copy link
Member

nikic commented Aug 13, 2020

I think it would be better to extract a version of php_hash_do_hash/_hmac without the zpp code (accepting the already parsed parameters and algorithm).

@kocsismate
Copy link
Member Author

The main reason why I chose this quick and dirty solution is that the two variants expect different number of parameters:

  • hash: (string $algo, string $data, bool $raw_output = true)
  • hash_hmac: (string $algo, string $data, string $key, bool $raw_output = true)

So similar to the imagefilter() function, it would be a "lie" to declare a non-variadic parameter list. Or do you think we should still go for it (using an extra parameter with UNKNOWN default value)? Or should the refactor happen on top of the stub changes in this PR?

Besides, the mhash*() functions should extint by PHP 9, so my incentive to do anything meaningful with mhash is a bit low. :/ Though, I can still try it if you think it's worth the effort.

@nikic
Copy link
Member

nikic commented Aug 13, 2020

@kocsismate As mhash does not accept the raw_output parameter, I think the signature should be pretty clean: ls|s

@kocsismate
Copy link
Member Author

@nikic I've just pushed a commit which refactors these functions. I quite much like the end result. Thanks for pushing for it :)

@kocsismate kocsismate changed the title Fix stub for mhash() Fix ZPP for mhash() Aug 13, 2020
size_t data_len;
zend_bool raw_output = raw_output_default;
static void php_hash_do_hash(
zval *return_value, zend_string *algo, char *data, size_t data_len, zend_bool raw_output, int isfilename
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
zval *return_value, zend_string *algo, char *data, size_t data_len, zend_bool raw_output, int isfilename
zval *return_value, zend_string *algo, char *data, size_t data_len, zend_bool raw_output, bool isfilename

Maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense for me, so I applied this change to master.

@php-pulls php-pulls closed this in f83368c Aug 14, 2020
@kocsismate kocsismate deleted the mhash-stub branch August 14, 2020 08:05
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.

2 participants