Support the actual #[\SensitiveParameter] attribute in stubs#8836
Support the actual #[\SensitiveParameter] attribute in stubs#8836kocsismate merged 6 commits intophp:masterfrom
#[\SensitiveParameter] attribute in stubs#8836Conversation
d121af5 to
0f9321a
Compare
0f9321a to
14b31f2
Compare
|
I suppose we can now also get rid of zend_mark_function_parameter_as_sensitive() and replace it by a proper mechanism akin to #8839 for arbitrary attributes. |
|
@bwoebi Was already working on it. Pushed now, PTAL 😃 |
| && $this->defaultValue === $other->defaultValue | ||
| && $this->isSensitive === $other->isSensitive; | ||
| && $this->defaultValue === $other->defaultValue; |
There was a problem hiding this comment.
Not sure about this part. I don't believe that attributes need to be part of equals(), as they are not part of the ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX stuff.
06ecf2b to
5820abe
Compare
|
Rebased for #8931. |
kocsismate
left a comment
There was a problem hiding this comment.
Looks good overall, I only had 2 minor questions/suggestions.
build/gen_stub.php
Outdated
|
|
||
| $code .= "\tzend_mark_function_parameter_as_sensitive($functionTable, \"" . $funcInfo->name->getNameForAttributes() . "\", $index);\n"; | ||
| foreach ($arg->attributes as $attribute) { | ||
| $code .= $attribute->generateCode("zend_add_parameter_attribute(zend_hash_str_find_ptr($functionTable, \"" . $funcInfo->name->getNameForAttributes() . "\", sizeof(\"" . $funcInfo->name->getNameForAttributes() . "\") - 1), $index", "{$funcInfo->getArgInfoName()}_arg{$index}", $allConstInfos); |
There was a problem hiding this comment.
I don't really like the fact that $funcInfo->getArgInfoName() is used for generating the attribute name, because the arginfo_ part is unnecessary (and I didn't realize for a while why it's there). Even though the format of $funcInfo->name->getMethodSynopsisFilename() looks a bit better, it would be weird to use this name here.. So either we should rename this method (getSnakeCasedName??) or just leave the name as-is.
There was a problem hiding this comment.
Thanks! $funcInfo->name->getMethodSynopsisFilename() is indeed much better. It's was hard for me to "discover" something appropriate to use and ->getArgInfoName() worked.
ext/openssl/openssl_arginfo.h
Outdated
| zend_mark_function_parameter_as_sensitive(CG(function_table), "openssl_pkey_derive", 1); | ||
| zend_mark_function_parameter_as_sensitive(CG(function_table), "openssl_spki_new", 0); | ||
|
|
||
| zend_string *attribute_name_SensitiveParameter_arginfo_openssl_x509_check_private_key_arg1 = zend_string_init("SensitiveParameter", sizeof("SensitiveParameter") - 1, 1); |
There was a problem hiding this comment.
I'm wondering if it would be beneficial to declare the different attribute names first and then use them in the zend_add_parameter_attribute() invocations. It would save quite a few lines when an extension has a lot of attributes with the same name
There was a problem hiding this comment.
It likely would, but that should probably be a follow-up, as it also affects the class attributes.
|
@kocsismate Thanks for the review! I've made the requested changes. Feel free to merge if you are happy with the new version. |
see #8689
see #8776