Skip to content

Obnoxious error in build/gen_stub.php "PHPDoc param type is unnecessary" #9183

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
dktapps opened this issue Jul 28, 2022 · 5 comments
Closed

Comments

@dktapps
Copy link
Contributor

dktapps commented Jul 28, 2022

Description

build/gen_stub.php bails out if it encounters doc comments like this:

     * @param int $size The maximum number of Workers this Pool can create
     * @param string $class The class for new Workers
     * @param array $ctor An array of arguments to be passed to new Workers

complaining that the @param annotations are unnecessary. It's my opinion that bailing out here is entirely unnecessary and annoying. A warning would be perfectly sufficient.
In addition, it makes it inconvenient to write doc comments in stubs.

@damianwadley
Copy link
Member

It's correct in that saying the parameter is a string is redundant, the problem is that it's not accounting for the fact that there's a description too. So IMO it should "error" if you say like @param string $class and nothing else, obviously allow @param string $class description here, then the error message should say that either you remove the tag or you add a description.

@dktapps
Copy link
Contributor Author

dktapps commented Jul 28, 2022

Yeah, the description part is fair enough. But I don't believe this should be a fatal error at all. I simply removed the check on my local because I was repurposing old stubs for header generation and didn't want to remove all the comments.

@nikic
Copy link
Member

nikic commented Jul 28, 2022

I think it's fine to just drop the check. For php-src usage we don't include docs in stubs, but it's reasonable that extensions want to do this, and I don't think this check is preventing any real bugs.

cc @kocsismate

@cmb69
Copy link
Member

cmb69 commented Jul 29, 2022

I think it's fine to just drop the check.

I agree, but we may consider to add check for a doc-type not matching the declared type (might be too complex, though), since this is a more serious issue anyway.

@kocsismate
Copy link
Member

@nikic Yes, you are right, the check is not really useful.

I agree, but we may consider to add check for a doc-type not matching the declared type (might be too complex, though), since this is a more serious issue anyway.

The idea is great, but implementing this would take way too much time (for me at least), so I'd spare my time for something else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants