Add visibility to test class methods

[costdev]

There are many tests that do not identify their visibility.

This ticket proposes to add public visibility to all test classes except those that begin with an underscore, suggesting private/protected visibility. This ticket proposes that these methods are given private visibility.

See results below.

---

Public Visibility: Attempt 1

/**
 * Methods that:
 *
 * 1. Begin with the start of a line, followed by one {TAB} character;
 * 2. may or may not be static;
 * 3. may or may not be passed by reference;
 * 4. do not have a leading underscore, suggesting public visibility.
 *
 * N.B. These are given PUBLIC visibility.
 */

Find:    ^(\t)(static )?(function )(&)?([^_])
Replace: $1$2public $3$4$5
Results: 3119 results in 314 files.
Gotchas: Many. Too many.

Public Visibility: Revision 1

/**
 * Methods that:
 *
 * 1. Begin with the start of a line, followed by one or more {TAB} characters;
 * 2. may or may not be static;
 * 3. may or may not be passed by reference;
 * 4. do not have a leading underscore, suggesting public visibility;
 * 5. match the prefix of test class methods (manual review of 3159 names to get prefixes).
 *
 * N.B. These are given PUBLIC visibility.
 */

Find:    ^(\t{1,})(static )?(function )(&)?([^_])(est|ata|ction|ilter|et|ear|lean|pply|init|s_unique|pload_dir|p_dropdown|p_mail|ub_reg|etrieve|ender|andle|ynamic|nstantiate|ate_validate_length|anitize|apture|eturn|o_customize|ustom|emove|verride|dd_custom|o_atom|o_rss|estable|ingle|upported|nvalid|nsupported|ntities|ocales|reaks|tf8|ig5|ncrement|dd_query_arg|arse|p_translate|ock_|pTear|he_posts|re_get|lain_|retty_|mpty_|upports|uthorless|o_example_shortcode|rant_unfiltered|evoke_unfiltered|isable)
Replace: $1$2public $3$4$5$6
Results: 3244 results in 328 files.
Gotchas: (nested functions)

- get_template_ids
- filter_allowed_block_types_my_editor
- filter_block_categories_my_editor
- filter_block_editor_settings_my_editor
- filter_remove_preload_paths
- filter_add_preload_paths
- find_and_normalize_template_by_id
- Numerous nested static anonymous functions, always indented by at least 3 {TAB} characters.

Public Visibility: Revision 2

/**
 * Methods that:
 *
 * 1. Begin with the start of a line, followed by one or two {TAB} characters (to exclude the static anonymous gotchas);
 * 2. may or may not be static;
 * 3. may or may not be passed by reference;
 * 4. do not have a leading underscore, suggesting public visibility;
 * 5. match the names of identified named gotchas.
 *
 * N.B. These are given PUBLIC visibility.
 */

Find:    ^(\t{1,2})(static )?(function )(&)?([^_])(?!et_template_ids|ilter_allowed_block_types_my_editor|ilter_block_categories_my_editor|ilter_block_editor_settings_my_editor|ilter_remove_preload_paths|ilter_add_preload_paths|ind_and_normalize_template_by_id)
Replace: $1$2public $3$4$5
Results: 3252 results in 328 files.
Gotchas: No gotchas identified.

---

Private Visibility: Attempt 1

/**
 * Methods that:
 *
 * 1. begin with the start of a line, followed by one or two or more {TAB} characters.
 * 2. may or may not be static;
 * 3. may or may not be passed by reference;
 * 4. have a leading underscore, suggesting private/protected visibility.
 *
 * N.B. These are given PRIVATE visibility.
 */

Find:    ^(\t{1,})(static )?(function )(&)?([_])
Replace: $1$2private $3$4$5
Results: 97 results in 40 files.
Gotchas: No gotchas identified.

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

[costdev]

There were new tests committed upstream that didn't identify their visibility just prior to me creating the initial PR, so this is still an ongoing standards issue.

We should try to find a way to ensure that all tests going forward have their visibility identified prior to commit. Ideally this would be added to the PHP coding standards, if they can detect class methods vs nested methods, so that committers don't have to spend the time to communicate this to contributors, which could create quite a delay.

If the standards can be updated to check for public, protected or private visibility, then leave it to the contributor to decide which to use (unit tests will fail if they choose wrong anyway), that would be great.

Pinging @hellofromTonya for PR review.

---

PHPCS Report on tests/phpunit/tests

See added rules in this comment.

# ./vendor/squizlabs/php_codesniffer/bin/phpcs tests/phpunit/tests/ --sniffs=Squiz.Scope.MethodScope,Squiz.WhiteSpace.ScopeKeywordSpacing --report=summary,source

PHP CODE SNIFFER VIOLATION SOURCE SUMMARY
----------------------------------------------------------------------
SOURCE                                                           COUNT
----------------------------------------------------------------------
Squiz.Scope.MethodScope.Missing                                  3349
----------------------------------------------------------------------
A TOTAL OF 3349 SNIFF VIOLATIONS WERE FOUND IN 1 SOURCE
----------------------------------------------------------------------

[costdev]

Regarding the coding standards point, I see that relevant rules were implemented in the WordPress-Extra ruleset as a result of ​this issue.

These rules are specifically related to this ticket:

        <rule ref="Squiz.Scope.MethodScope"/>
        <rule ref="Squiz.WhiteSpace.ScopeKeywordSpacing"/>

Given that this ticket aims to resolve missing scope, it would be a shame to miss the opportunity to move these into the WordPress-Core ruleset, targeting the tests/phpunit/tests/ directory, so that we don't have to repeat this task in future.

Similar to this (with some review of the paths):

        <!-- Exclude some directories from scope sniffs. -->
        <rule ref="Squiz.Scope.MethodScope">
                <exclude-pattern>/src/*</exclude-pattern>
                <exclude-pattern>/build/*</exclude-pattern>
                <exclude-pattern>/node_modules/*</exclude-pattern>
                <exclude-pattern>/vendor/*</exclude-pattern>
                <exclude-pattern>/tools/*</exclude-pattern>
                <exclude-pattern>/tests/e2e/*</exclude-pattern>
                <exclude-pattern>/tests/gutenberg/*</exclude-pattern>
                <exclude-pattern>/tests/qunit/*</exclude-pattern>
                <exclude-pattern>/tests/phpunit/build/*</exclude-pattern>
                <exclude-pattern>/tests/phpunit/data/*</exclude-pattern>
                <exclude-pattern>/tests/phpunit/includes/*</exclude-pattern>
        </rule>
        <rule ref="Squiz.WhiteSpace.ScopeKeywordSpacing">
                <exclude-pattern>/src/*</exclude-pattern>
                <exclude-pattern>/build/*</exclude-pattern>
                <exclude-pattern>/node_modules/*</exclude-pattern>
                <exclude-pattern>/vendor/*</exclude-pattern>
                <exclude-pattern>/tools/*</exclude-pattern>
                <exclude-pattern>/tests/e2e/*</exclude-pattern>
                <exclude-pattern>/tests/gutenberg/*</exclude-pattern>
                <exclude-pattern>/tests/qunit/*</exclude-pattern>
                <exclude-pattern>/tests/phpunit/build/*</exclude-pattern>
                <exclude-pattern>/tests/phpunit/data/*</exclude-pattern>
                <exclude-pattern>/tests/phpunit/includes/*</exclude-pattern>
        </rule>

---

If there's a desire to extend this for the whole of core, I'm happy to:

1. open an additional ticket, so that this one is essentially a dry-run for core.
2. add scope from private to protected to public until all tests pass.
3. open a PR when this is ready for review.

Assumptions:

• We'd need to update the GitHub actions at the same time as merging the change.
• A re-run of GitHub actions would possibly need to be forced on open PRs at the same time so that all of these can be updated before being merged.
• A core merge is best done just after a major release.

---

PHPCS Report on src

# ./vendor/squizlabs/php_codesniffer/bin/phpcs src/ --sniffs=Squiz.Scope.MethodScope,Squiz.WhiteSpace.ScopeKeywordSpacing --report=summary,source

PHP CODE SNIFFER VIOLATION SOURCE SUMMARY
----------------------------------------------------------------------
SOURCE                                                           COUNT
----------------------------------------------------------------------
Squiz.Scope.MethodScope.Missing                                  99
----------------------------------------------------------------------
A TOTAL OF 99 SNIFF VIOLATIONS WERE FOUND IN 1 SOURCE
----------------------------------------------------------------------

[jrf]

@costdev I fully support moving those sniffs from WPCS Extra to Core. Probably should also include the Squiz.WhiteSpace.ScopeKeywordSpacing and the PSR2.Methods.MethodDeclaration sniff.

To allow for a PR to that effect in ​the WPCS repo, the ​Core coding standards handbook page would ​need to be updated first.

Once those steps have been taken, we can do an auto-fixer run with PHPCS to patch the whole of Core in one go.

IMO no exclusion configuration should be introduced for these rules.

The auto-fixer can be run selectively to only apply these rules like so:

phpcbf --sniffs=Squiz.Scope.MethodScope,PSR2.Classes.PropertyDeclaration,Squiz.WhiteSpace.ScopeKeywordSpacing,PSR2.Methods.MethodDeclaration

A visual check of the applied fixes before commit can then weed out those methods and properties which should become private instead of public.
I could even create a slightly adjusted one-time use only sniff, to respect the underscore prefixes.

IMO no separate tickets are needed for these fixes. Ticket #53359 is an umbrella ticket under which these patches can go in.

Patches already prepared (with PHPCS) to fix (subsets of) this issue in the current codebase do not need to wait for the rules to be adjusted in the handbook/WPCS, these can already be committed in anticipation of the change to the handbook.

[costdev]

@jrf To allow for a PR to that effect in the WPCS repo, the Core coding standards handbook page would need to be updated first.

I've looked into adding the four rules mentioned, but I'm not confident that I could write the updates to the handbook in an easily digestible way.

IMO no exclusion configuration should be introduced for these rules.

Agreed - the more consistent the better!

So from here, shall we consider this ticket's scope to apply not only to tests, but to all of Core?

• If so, I'll update the PR on this ticket with the outcome of the auto-fixer across Core (see below for more detail on the auto-fixer) and it'll be ready for review.
• If not, then the PR on this ticket is ready for review and I'll open another PR on #53359 to handle the rest of Core.

Given that we're not going to be adding exclusion configuration, when updating the whole codebase are we also fixing files in tests/phpunit/includes for example?

---

Auto-Fixes

I ran into an issue with the auto-fixer - it doesn't appear to have fixes for Squiz.Scope.MethodScope or PSR2.Classes.PropertyDeclaration. That or I'm missing something...

Anyway, with a lack of awareness of any auto-fixes for these, I wrote some (added below for reference). For our case, they add private if an underscore is found, otherwise public.

From there it was a case of doing a PHPUnit run and adjusting the private scopes to protected or public until the tests passed.

Spoiler Alert: There were four, all in wp-db.php and all had to be made public in the end.

---

Sniff Fixes

Squiz.Scope.MethodScope

See ​here.

PSR2.Classes.PropertyDeclaration

See ​here.

[jrf]

Replying to costdev:

@jrf To allow for a PR to that effect in the WPCS repo, the Core coding standards handbook page would need to be updated first.

I've looked into adding the four rules mentioned, but I'm not confident that I could write the updates to the handbook in an easily digestible way.

No worries. We'll sort that out when actioning the ​WPCS update for modern PHP, though that does mean that the sniffs won't be moved (from Extra to Core) until that's all sorted.

See the "Visibility should always be declared" section in that dev note for more information.

IMO no exclusion configuration should be introduced for these rules.

Agreed - the more consistent the better!

So from here, shall we consider this ticket's scope to apply not only to tests, but to all of Core?

Sounds good to me. Might be an idea to post a link to this ticket in #53359 for visibility.

• If so, I'll update the PR on this ticket with the outcome of the auto-fixer across Core (see below for more detail on the auto-fixer) and it'll be ready for review.
• If not, then the PR on this ticket is ready for review and I'll open another PR on #53359 to handle the rest of Core.

As this is potentially a huge patch, it might make for easier reviewing and quicker commits to submit this in chunks, wp-includes, wp-admin, tests/phpunit/tests etc.

Given that we're not going to be adding exclusion configuration, when updating the whole codebase are we also fixing files in tests/phpunit/includes for example?

Absolutely, but please be careful not to break BC. When in doubt, always use public. To quote myself:

If any visibility modifiers are missing in WordPress Core code, they should be set to public so as not to break backward-compatibility.

Source: ​WPCS update for modern PHP

Auto-Fixes

I ran into an issue with the auto-fixer - it doesn't appear to have fixes for Squiz.Scope.MethodScope or PSR2.Classes.PropertyDeclaration. That or I'm missing something...

Darn! Hmm.. I think that may have been over-cautiousness on the side of PHPCS.

Anyway, with a lack of awareness of any auto-fixes for these, I wrote some (added below for reference). For our case, they add private if an underscore is found, otherwise public.

For the actual tests, I'm fine with making the underscore prefixed methods private, everywhere else, they should still be made public so as not to break BC.

While the methods may have been intended for WP internal use only, this doesn't mean that has been respected by all plugins/themes.

No visibility means that the previous behaviour was public, so the method visibility should remain public.

From there it was a case of doing a PHPUnit run and adjusting the private scopes to protected or public until the tests passed.

Spoiler Alert: There were four, all in wp-db.php and all had to be made public in the end.

👍🏻

---

Sniff Fixes

Have you got a branch somewhere in a fork with these changes ? I'd recommend a few small tweaks and can push those up to your fork if you like.

[costdev]

No worries. We'll sort that out when actioning the ​WPCS update for modern PHP, though that does mean that the sniffs won't be moved (from Extra to Core) until that's all sorted. See the "Visibility should always be declared" section in that dev note for more information.

Great, thanks for that!

Sounds good to me. Might be an idea to post a link to this ticket in #53359 for visibility.

Done!

As this is potentially a huge patch, it might make for easier reviewing and quicker commits to submit this in chunks, wp-includes, wp-admin, tests/phpunit/tests etc.

No problem - I'll split this into separate PRs for easier reviewing.

Absolutely, but please be careful not to break BC. When in doubt, always use public.

Noted!

Have you got a branch somewhere in a fork with these changes ? I'd recommend a few small tweaks and can push those up to your fork if you like.

See ​here.

Apply PHPCS fixes to Core.

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

Apply PHPCS fixes to tests/includes files.

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

Apply PHPCS fixes to tests/phpunit/tests root files.

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

Apply PHPCS fixes to tests/phpunit/tests A-B-C-D files.

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

Appy PHPCS fixes to tests/phpunit/tests E-F-G-H files.

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

Apply PHPCS fixes to tests/phpunit/tests i-L-M-O files.

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

Apply PHPCS fixes to tests/phpunit/tests P-Q-R-S files.

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

Apply PHPCS fixes to tests/phpunit/tests T-U-W-X files.

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

[costdev]

I've opened separate PRs to make the reviewing process easier, so the initial PR on this ticket can be ignored for now (keeping it open as a reference point until the ticket's resolved).

All PRs pass CI and are ready for review.

[costdev]

PR Update

Approved

• PR 1713

Under self-review - No reviewer assigned

• PR 1715
• PR 1720
• PR 1721

Under self-review - To be reviewed by @jrf when they are ready.

• PR 1717
• PR 1718
• PR 1719

I'll post an update when these are ready for review.

@hellofromTonya

[costdev]

PR Update

Approved

• ​PR 1713

Ready for review - No reviewer assigned.

• ​PR 1715
• ​PR 1720
• ​PR 1721

Ready for review - To be reviewed by @jrf.

• ​PR 1717
• ​PR 1718
• ​PR 1719

@hellofromTonya

[hellofromTonya]

Marking commit for ​PR 1713 and currently reviewing for commit.

[hellofromTonya]

In 51919:

Coding Standards: Add public visibility to methods in src directory.

This commit adds the public visibility keyword to each method which did not have an explicit visibility keyword.

Why public?

With no visibility previously declared, these methods are implicitly public and available for use. Changing them to anything else would be a backwards-compatibility break.

Props costdev, jrf.
See #54177.

Committed via changeset 51919.

[hellofromTonya]

Removing commit keyword as the "ready for commit" PR has been committed.

Thank you @costdev and @jrf for your contributions!

@costdev Can you rebase with master to resolve these conflicts please? Then it'll be ready to go.

@costdev Can you rebase with master please to resolve the merge conflicts?

@hellofromtonya There was a failure on ​WP_REST_Block_Directory_Controller_Test::test_get_items, but passes for me locally. Might just need a re-run.

[hellofromTonya]

In 52009:

Coding Standards: Add public visibility to methods in tests/phpunit/includes/.

This commit adds the public visibility keyword to each method which did not have an explicit visibility keyword.

Why public?

With no visibility previously declared, these methods are implicitly public and available for use. As these are part of the WordPress testing framework (for Core and extenders), changing them to anything else would be a backwards-compatibility break.

Props costdev, jrf, hellofromTonya.
See #54177.

Committed via changeset https://core.trac.wordpress.org/changeset/52009.

PRs 1715 through 1721 replace this PR.

[hellofromTonya]

All PRs have been thoroughly reviewed and approved. Marking for commit. My plan is to combine all tests/phpunit/tests/ PRs as one commit.

[hellofromTonya]

In 52010:

Coding Standards: Add visibility to methods in tests/phpunit/tests/.

Adds a public visibility to test fixtures, tests, data providers, and callbacks methods.

Adds a private visibility to helper methods within test classes.

Renames callbacks and helpers that previously started with a _ prefix. Why? For consistency and to leverage using the method visibility. Further naming standardizations is beyond the scope of this commit.

Props costdev, jrf, hellofromTonya.
Fixes #54177.

Committed via https://core.trac.wordpress.org/changeset/52010.

Committed via https://core.trac.wordpress.org/changeset/52010.

Committed via https://core.trac.wordpress.org/changeset/52010.

Committed via https://core.trac.wordpress.org/changeset/52010.

Committed via https://core.trac.wordpress.org/changeset/52010.

Committed via https://core.trac.wordpress.org/changeset/52010.

[swissspidy]

#43424 was marked as a duplicate.