Skip to content

Conversation

@costdev
Copy link
Contributor

@costdev costdev commented Sep 27, 2021

Coding Standards: Add visibility to methods in tests/phpunit/tests/ A-B-C-D files.

"Visibility should always be declared"
See: https://make.wordpress.org/core/2020/03/20/updating-the-coding-standards-for-modern-php/

This commit applies the following sniffs to files in tests/phpunit/tests/ A-B-C-D directories:

  • Squiz.Scope.MethodScope
  • PSR2.Methods.MethodDeclaration
  • PSR2.Classes.PropertyDeclaration
  • Squiz.WhiteSpace.ScopeKeywordSpacing

For most methods, these now indicate public visibility to avoid breaking backwards compatibility.
Any remaining methods now indicate private visibility where appropriate.

Additional fixups in this PR:

  1. Methods touched by this PR have had unnecessary underscores removed from their names.

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

@jrfnl jrfnl self-assigned this Oct 16, 2021
@costdev costdev force-pushed the #54177-tests-a-b-c-d branch from 19fb30b to d6a3428 Compare October 17, 2021 18:44
costdev added 2 commits October 18, 2021 02:43
Note:
In `tests/phpunit/tests/admin/includesTheme.php`:
The `_theme_root()` method is used as a callback in `set_up()`.

This has now been appropriately prefixed with `filter_` instead of `_`.
@costdev costdev force-pushed the #54177-tests-a-b-c-d branch from d6a3428 to ff9b127 Compare October 18, 2021 01:46
@costdev costdev changed the title Add PHPCS fixes to tests/phpunit/tests A-B-C-D files. Coding Standards: Add visibility to methods in tests/phpunit/tests/ A-B-C-D files. Oct 18, 2021
@hellofromtonya
Copy link
Contributor

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

class Tests_Functions_wpScriptTag extends WP_UnitTestCase {

function get_script_tag_type_set() {
public function get_script_tag_type_set() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this test needs a test_ in front of it for it run. Outside the scope of this ticket. But it needs to be fixed.

Copy link
Contributor Author

@costdev costdev Nov 4, 2021

Choose a reason for hiding this comment

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

👍 Also needs:

  1. A $message value for the two assertions or, ideally, a data provider.
  2. A @covers tag for ::wp_get_script_tag.
  3. Some work on the test as both assertions currently fail for the following reasons:

Test 1 - wp_get_script_tag() returns the attributes in the order they were supplied. The expected value has the attributes in a different order than it supplied to wp_get_script_tag().

Test 2 - wp_get_script_tag() calls wp_sanitize_script_attributes(). For boolean attributes, this returns attribute="attribute". The test supplies 'nomodule' => true and tests for nomodule, but gets nomodule="nomodule" instead. IMO, this should be part of a wider discussion in Core about the usage of attribute="attribute". However, that is beyond the scope of fixing that test, which just needs some minor corrections in the expected values.

Copy link
Contributor

@hellofromtonya hellofromtonya left a comment

Choose a reason for hiding this comment

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

Well done @costdev. Ready for commit.

@hellofromtonya
Copy link
Contributor

@costdev costdev deleted the #54177-tests-a-b-c-d branch September 19, 2022 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants