Skip to content

[VarDumper] Fix ResourceCaster deprecation messages #60810

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

Merged

Conversation

derrabus
Copy link
Member

Q A
Branch? 7.3
Bug fix? yes
New feature? no
Deprecations? no
Issues N/A
License MIT

We pass more arguments to trigger_deprecation() than the deprecation message has placeholders.

@carsonbot carsonbot added this to the 7.3 milestone Jun 17, 2025
@carsonbot carsonbot changed the title Fix ResourceCaster deprecation messages [VarDumper] Fix ResourceCaster deprecation messages Jun 17, 2025
@@ -29,7 +29,7 @@ class ResourceCaster
*/
public static function castCurl(\CurlHandle $h, array $a, Stub $stub, bool $isNested): array
{
trigger_deprecation('symfony/var-dumper', '7.3', 'The "%s()" method is deprecated without replacement.', __METHOD__, CurlCaster::class);
trigger_deprecation('symfony/var-dumper', '7.3', 'The "%s()" method is deprecated without replacement.', __METHOD__);
Copy link
Member

Choose a reason for hiding this comment

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

I think we should not even use the extra arguments there but use an explicit \sprintf call instead, which can be optimized by OPCache at compile time on recent PHP versions as we only use %s placeholders (the optimization supports %s and %d).

Copy link
Member

Choose a reason for hiding this comment

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

And as another benefit, static analysis tools are able to detect such issues about mismatch for the number of parameters.

Copy link
Member

Choose a reason for hiding this comment

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

Let me ignore this concern and merge: the patch is using the API the expected way and the perf topic looks minor to me.

@nicolas-grekas
Copy link
Member

Thank you @derrabus.

@nicolas-grekas nicolas-grekas merged commit 89c0b76 into symfony:7.3 Jun 18, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants