Added deprecated warning to deprecated functions missing it. #3375
Conversation
| protected $expected_deprecated = array( | ||
| 'wp_slash_strings_only', | ||
| 'addslashes_strings_only', | ||
| ); |
There was a problem hiding this comment.
@jrfnl, @hellofromtonya All but one of the test methods in this class will expect deprecation notices for these functions.
By defining them here, it means that $this->expected_deprecated = array() can be called in the one test that doesn't expect these deprecation notices to occur, and doesn't apply to later tests in the class.
While this makes sense, I haven't seen this approach used in Core's test suite before, so I'm pinging for your thoughts. Note: This is also done in another file in this PR.
There was a problem hiding this comment.
Just my two pennies:
I'm not keen on this as it obfuscates what's going on and effectively "hides" that the deprecation is being expected and tested for every single function (save one).
While it works, it requires a deep understanding of the test suite framework to understand what's going on, which makes these tests error prone going forward.
There was a problem hiding this comment.
As a frustrated side-note (not to be actioned in this PR): why the heck is this even possible and why is the $expected_deprecated property in the WP_UnitTestCase_Base class protected instead of private ???
There was a problem hiding this comment.
Thanks @jrfnl! I agree - While more verbose to add this to each of the related test methods:
* @expectedDeprecated wp_slash_strings_only
* @expectedDeprecated addslashes_strings_only(cont.) it maintains consistency with the rest of the test suite, and makes it easier to read each test method in isolation and know exactly what should be expected without needing to review the rest of the test class.
As a frustrated side-note (not to be actioned in this PR): why the heck is this even possible and why is the
$expected_deprecatedproperty in theWP_UnitTestCase_Baseclassprotectedinstead ofprivate???
Because you're too happy and it's important that codebases drop in these little easter eggs to disrupt the flow. 😂
Added missing
_deprecated_functioncall in deprecated functions missing it.Trac ticket: https://core.trac.wordpress.org/ticket/56589
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.