Skip to content

Conversation

@jeromegamez
Copy link
Contributor

@jeromegamez jeromegamez commented Oct 26, 2025

When running PHPStan with the https://github.com/phpstan/phpstan-deprecation-rules/ extension installed, the whole method is seen as deprecated, leading to violations like in https://github.com/kreait/firebase-php/actions/runs/18822790410/job/53700886141

Call to deprecated method __construct() of class Google\Cloud\Storage\StorageClient:
This option is being deprecated because of a potential security risk.

When hovering about the constructor in PHPStorm, the constructor itself is marked as deprecated as well (here in the FirestoreClient):

CleanShot 2025-10-26 at 22 56 23@2x

I don't know if this an authoritative source, but https://docs.phpdoc.org/guide/references/phpdoc/tags/deprecated.html mentions that @deprecated applies to structural elements - I'm not aware that one is supposed to use it to deprecate fields in an array with this.

:octocat:

PS: I couldn't find a source for @type "being a thing" 🙈

@jeromegamez jeromegamez requested review from a team as code owners October 26, 2025 22:01
@jeromegamez jeromegamez force-pushed the chore/remove-deprecated-annotation-on-array-options branch from bdbabbf to ac4ee40 Compare October 28, 2025 10:39
@bshaffer
Copy link
Contributor

When running PHPStan... the whole method is seen as deprecated

Well that's unfortunate. I checked using IntelliPHPence, and didn't have this issue. I wonder if there's a way to fix this for PHPStan without removing it, as i really would like to keep it there.

PS: I couldn't find a source for @type "being a thing" 🙈

PHPStan is only one standardization around documenting PHP methods/array shapes, and at the time this library was written, the documentation standard being used here (@type) was all we could find.

* Only valid for requests sent over REST.
* @type array $keyFile [DEPRECATED]
* @deprecated This option is being deprecated because of a potential security risk.
* This option is being deprecated because of a potential security risk.
Copy link
Contributor

@bshaffer bshaffer Oct 28, 2025

Choose a reason for hiding this comment

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

Can you try using this syntax and see if it works (prefixing @deprecated with //)? This should tell PHPStan to ignore that line (e.g. treat it as a comment rather than parse the tag)

Suggested change
* This option is being deprecated because of a potential security risk.
* // @deprecated
* This option is being deprecated because of a potential security risk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, this didn't change the report (I think because it's just™ a comment within a comment).

In a project, you can add this to silence the error:

/* @phpstan-ignore method.deprecated */
$client = new FirestoreClient($config);

(only when they use https://github.com/phpstan/phpstan-deprecation-rules )

@jeromegamez
Copy link
Contributor Author

jeromegamez commented Oct 28, 2025

When running PHPStan... the whole method is seen as deprecated

Well that's unfortunate. I checked using IntelliPHPence, and didn't have this issue. I wonder if there's a way to fix this for PHPStan without removing it, as i really would like to keep it there.

It's only when using https://github.com/phpstan/phpstan-deprecation-rules - "vanilla" PHPStan doesn't report this. People can still ignore this specific error in their own projects, they'd have to add a /* @phpstan-ignore method.deprecated */ where they call the constructor.

But aside from PHPStan, my IDE shows the constructor as deprecated as well (see screenshot above) - do you know if other IDEs/Doc tools can parse the docblock as intendend and only deprecate the array key? Otherwise I'd still like to suggest removing it and keeping only the [Deprecated] as an indicator. IDEs do parse the annotation, and - in the case of PHPStorm - it looks as if they only check if @deprecated is somewhere inside the docblock, not explicitly at the "root" level

/**
 * @deprecated
 */

@jeromegamez jeromegamez force-pushed the chore/remove-deprecated-annotation-on-array-options branch from ac4ee40 to 0b0c628 Compare October 28, 2025 22:26
@jeromegamez jeromegamez force-pushed the chore/remove-deprecated-annotation-on-array-options branch from 0b0c628 to 8874e02 Compare November 1, 2025 16:40
jeromegamez added a commit to kreait/firebase-php that referenced this pull request Nov 1, 2025
The deprecation error has been introduced with the changes in
googleapis/google-cloud-php#8617.

Until/Unless googleapis/google-cloud-php#8689 or
something similar is implemented, we need to ignore the error here.
jeromegamez added a commit to kreait/firebase-php that referenced this pull request Nov 1, 2025
The deprecation error has been introduced with the changes in
googleapis/google-cloud-php#8617.

Until/Unless googleapis/google-cloud-php#8689 or
something similar is implemented, we need to ignore the error here.
@bshaffer
Copy link
Contributor

But aside from PHPStan, my IDE shows the constructor as deprecated as well

Ahh you're right, I missed that. I don't like that at all

I'd still like to suggest removing it and keeping only the [Deprecated] as an indicator.

This seems like a good idea

@bshaffer bshaffer added the next release PRs to be included in the next release label Nov 14, 2025
@Hectorhammett
Copy link
Collaborator

Makes perfect sense. I wish there was a better way to really alert the end user with the deprecated parameter, but I feel like this is a weakness of using untyped arrays for the options. Leaving the DEPRECATED message seems like a good compromise to me.

@bshaffer bshaffer merged commit 77185ff into googleapis:main Nov 14, 2025
28 of 29 checks passed
@jeromegamez jeromegamez deleted the chore/remove-deprecated-annotation-on-array-options branch November 14, 2025 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

next release PRs to be included in the next release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants