Skip to content

Add support for sensitive parameters in stubs #8689

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
Merged
merged 2 commits into from
Jun 4, 2022
Merged

Add support for sensitive parameters in stubs #8689

merged 2 commits into from
Jun 4, 2022

Conversation

kocsismate
Copy link
Member

To make the implementation of #8352 easier.

TimWolla
TimWolla reviewed Jun 2, 2022
View reviewed changes
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

You'll likely need to cherry-pick this e7ff926 commit from my PR.

TimWolla
TimWolla approved these changes Jun 2, 2022
View reviewed changes
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! This LGTM as far as I am qualified to tell.

TimWolla
TimWolla requested changes Jun 2, 2022
View reviewed changes
TimWolla
TimWolla requested changes Jun 2, 2022
View reviewed changes
@kocsismate kocsismate force-pushed the stub-attributes branch 2 times, most recently from d03edf8 to 197faf4 Compare June 3, 2022 07:21
TimWolla
TimWolla approved these changes Jun 3, 2022
View reviewed changes
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

Verified that it does the job with #8352.

@bwoebi
Copy link
Member

bwoebi commented Jun 3, 2022

Is there a particular reason why we went with a doc-comment instead of #[SensitiveParameter] annotation?

@kocsismate
Copy link
Member Author

kocsismate commented Jun 3, 2022 •
edited
Loading

Is there a particular reason why we went with a doc-comment instead of #[SensitiveParameter] annotation?

No, not really. It's just that we have been using phpdoc and attributes don't offer too much value in this case as we have to process stubs via PHP-Parser anyway. Also, gen_stub.php is compatible with PHP 7.1, so we cannot use more modern features until we settle on requiring PHP 8.0 at least.

@kocsismate kocsismate merged commit debd38f into php:master Jun 4, 2022
@kocsismate kocsismate deleted the stub-attributes branch June 4, 2022 16:15
iluuu1994
iluuu1994 reviewed Jun 4, 2022
View reviewed changes
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. They're just minor comments.

build/gen_stub.php
@@ -4090,7 +4178,7 @@ function initPhpParser() {
installPhpParser($version, $phpParserDir);
}

spl_autoload_register(function(string $class) use($phpParserDir) {
spl_autoload_register(function(string $class) use ($phpParserDir) {
Copy link
Member

Choose a reason for hiding this comment

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

Since you made the one above static, this one can also be static.

build/gen_stub.php
@@ -970,6 +983,10 @@ public function getMethodSynopsisFilename(): string {
return implode('_', $this->name->parts);
}

public function getAttributeName(): string {
Copy link
Member

@iluuu1994 iluuu1994 Jun 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This name is a bit confusing. It's not the attribute name but the canonicalized (ie lowercased) function/method name to attach the given attribute to.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you are right. However, I think it's better if the method name reflects the use-case (similarly togetMethodSynopsisFilename() and getArgInfoName()) instead of using "canonicalized". That's why I changed getAttributeName() for getNameForAttributes() in b2ed625. Hopefully, it addresses your comments and make intention more clear.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, perfect 🙂

build/gen_stub.php
case 'prefer-ref':
case 'sensitive-param':
$varName = $tag->getVariableName();
if (!isset($paramMeta[$varName])) {
Copy link
Member

@iluuu1994 iluuu1994 Jun 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The isset block is redundant, $paramMeta[$varName][$tag->name] = true; will automatically create the outer array.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I also noticed this, but didn't want to touch it as this piece of code was originally written by Nikita

@TimWolla TimWolla mentioned this pull request Jun 21, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iluuu1994 iluuu1994 iluuu1994 left review comments

@TimWolla TimWolla TimWolla approved these changes

@cmb69 cmb69 Awaiting requested review from cmb69

@bwoebi bwoebi Awaiting requested review from bwoebi

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

Successfully merging this pull request may close these issues.
4 participants
@kocsismate @bwoebi @TimWolla @iluuu1994