[PHP 8.2] Fix deprecated `${var}` string interpolation syntax

[ayeshrajans]

PHP 8.2 deprecates string interpolation patterns that place the dollar sign outside the curly braces. This fixes such patterns by replacing them with proper curly braced patterns.

See:

• ​PHP.Watch: `${var}` string interpolation deprecated
• ​wiki.php.net RFC

PHP 8.2 deprecates string interpolation patterns that place the dollar sign outside the curly braces. This fixes such patterns by replacing them with proper curly braced patterns.

See:

• ​PHP.Watch: `${var}` string interpolation deprecated
• ​wiki.php.net RFC

Trac ticket: https://core.trac.wordpress.org/ticket/55787

@Ayesh Are there any unit tests covering this code ? Would be good to make sure that these changes can't be reverted without tests failing.

@Ayesh In the mean time, I've created a sniff for PHPCompatibility to scan for this (not public yet) and running that sniff over WP Core discovered one more issue in the tests:

​https://github.com/WordPress/wordpress-develop/blob/trunk/tests/phpunit/tests/formatting/convertSmilies.php#L20

LINE 20: WARNING Using ${includes_path} in strings is deprecated since PHP 8.2, use {$var} instead.

Want to include fixing that in this patch ? As that is something _in_ the tests, obviously no test is needed for that fix and based on what the test where I encountered the issue is testing, I don't think this change will diminish the value of the test.

Thanks a lot for the sniff and pointing out that missing change. I rebased and force-pushed with the change in convertSmilies test as well.

As for the tests, my thinking was that the test runner log should indicate any deprecation notices even after PHPUnit defaults to not throw an exception on deprecations. Do you think we should add tests to ensure the variables are properly inserted? I don't know if PHPUnit has any assertion for the opposite of expectDeprecation, or a way to selectively enable the convertDeprecationsToExceptions option just for one test, but we can certainly test the output.

Thanks a lot for the sniff and pointing out that missing change. I rebased and force-pushed with the change in convertSmilies test as well.

👍🏻

As for the tests, my thinking was that the test runner log should indicate any deprecation notices even after PHPUnit defaults to not throw an exception on deprecations.

I'm aware of the change in PHPUnit 10 and to be honest, I think it's a problematic change. Either way, as PHPUnit 9.x will support PHP 8.2, it's not something we have to solve or even worry about with any urgency.

As for showing all deprecation notices/not converting to exception, there's two reasons we expressly don't do that at this time:

1. There are tests in the WP test suite which actually _expect_ deprecation notices, those break when convertDeprecationsToExceptions is set to false.
2. WordPress has too many dependencies (plugins/themes) and the deprecations coming from WP itself would hinder them in finding their own incompatibilities, so WP needs to (try to) be ready early, no matter that the general sentiment in the PHP world is that deprecations don't matter.

And let's not even mention the amount of trac tickets which are opened by users seeing deprecation notices and panicking, even when those are already known.

Also see the discussion in https://core.trac.wordpress.org/ticket/54183

Do you think we should add tests to ensure the variables are properly inserted? I don't know if PHPUnit has any assertion for the opposite of expectDeprecation, or a way to selectively enable the convertDeprecationsToExceptions option just for one test, but we can certainly test the output.

As far as I'm aware PHPUnit does not have way to assert the opposite of expectDeprecation or to selectively enable the option, but it will fail a test which causes an unexpected exception, so any test which hits the affected code would do, as that test would start failing when a deprecation is thrown (though a "proper" test testing the functionality is of course preferred).

I've already been running the tests against PHP 8.2 and while in the "bootstrap", both (src) deprecations are hit, there aren't any dedicated tests for the affected methods which are failing (even though I can see the one in po.php being touched during the test run).

................................EEE.EEESEEEEEEE............  7493 / 13954 ( 53%)
Deprecated: Using ${var} in strings is deprecated, use {$var} instead in `/path/to/src/wp-includes/pomo/po.php on line 129
EEEEEEEEEEE......EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE  7552 / 13954 ( 54%)

I've done some digging in the tests.

### Regarding the change in comment-template.php:

The Tests_Comment_GetCommentReplyLink::test_get_comment_reply_link_should_include_post_permalink() test actually hits the code, though I can't seem to get the deprecation notice to show when running the tests on PHP 8.2.

The deprecation notice _does_ show during the test bootstrapping and I can confirm that the fix applied here removes that deprecation notice:

Installing...                                                                                                                                             
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml                                                                            
                                                                                                                                                          
Deprecated: Using ${var} in strings is deprecated, use {$var} instead in path/to/src/wp-includes/comment-template.php on line 1741    
Not running ajax tests. To execute these, use --group ajax.                                                                                               
Not running ms-files tests. To execute these, use --group ms-files.                                                                                       

### Regarding the change in pomo/po.php

I'm seeing something similar there - the deprecation does not show for the tests in the Tests_POMO_PO class, but it shows _before_ those tests are run.

### Further investigation

This lead me to investigate how PHP itself throws the deprecation and it turns out that the deprecation is thrown on _file load_ (compile), not when the code is actually _run_.

I confirmed this via ​https://3v4l.org/Tj2U5/rfc#vgit.master

### Conclusion

For both changes, tests are actually in place which hit the code.
No tests can be added to safeguard the deprecation as the deprecation is generated at compile time, not when the code is run.

In other words: this patch is 💯 good to go. Let's get this merged!

[jrf]

Marking as ready for commit after the additional investigation into tests covering the changes.

[SergeyBiryukov]

In 54134:

Code Modernization: Replace deprecated string interpolation patterns.

PHP 8.2 deprecates string interpolation patterns that place the dollar sign outside the curly braces:

echo "Hello ${name}";

This commit fixes such patterns by replacing them with proper curly braced patterns:

echo "Hello {$name}";

This addresses Deprecated: Using ${var} in strings is deprecated, use {$var} instead notices when running tests on PHP 8.2.

References:

• ​PHP.Watch: PHP 8.2: ${var} string interpolation deprecated
• ​PHP RFC: Deprecate ${} string interpolation

Follow-up to [10584], [31733], [42360], [53922].

Props ayeshrajans, jrf.
Fixes #55787.

Thanks for the PR! Merged in r54134.

Thanks for getting this merged @SergeyBiryukov !