Skip to content
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

Add wrapper wp_get_object_vars() #3607

Open
wants to merge 10 commits into
base: trunk
Choose a base branch
from

Conversation

cadic
Copy link

@cadic cadic commented Nov 11, 2022

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


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.

Copy link
Contributor

@costdev costdev left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @cadic! I've left some feedback in this review to tidy things up and meet Core standards/convention. 🙂 Feel free to ping me for a re-review.

* @since 6.3.0
*
* @param object $object An object instance.
* @return array
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @return array
* @return array The properties of the given object.
Comment on lines +68 to +69
* @dataProvider data_test_wp_comment_null_byte
* @ticket 52738
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @dataProvider data_test_wp_comment_null_byte
* @ticket 52738
* @ticket 52738
*
* @covers WP_Comment::__construct
*
* @dataProvider data_test_wp_comment_null_byte
*
* @param stdClass $comment_data The comment's data.
* @param string $expected The expected comment content.
  • See this ticket for the order of annotations in the test suite.
  • A @covers annotation should be used.
  • Test method parameters should be documented.

🔢 Applies elsewhere in the PR.

$this->assertSame( $comment->comment_content, $expected );
}

public function data_test_wp_comment_null_byte() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public function data_test_wp_comment_null_byte() {
/**
* Data provider.
*
* @return array[]
*/
public function data_test_wp_comment_null_byte() {

🔢 Applies elsewhere in the PR.

// '8)' => "\xf0\x9f\x98\x8e",
// '8)' => "\xf0\x9f\x98\x8e",
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like an unrelated change.

Comment on lines +5971 to +5972
/*
First we check if the DOMDocument class exists. If it does not exist, then we cannot
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like an unrelated change.


public function data_test_wp_comment_null_byte() {
return array(
array(
Copy link
Contributor

Choose a reason for hiding this comment

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

Datasets should have named keys so that it's immediately clear what's being tested, and that passing --testdox to PHPUnit makes it clear what is being tested.

For example:

  • 'a key that is only a NUL byte'
  • 'a key starting with a NUL byte'
  • 'a key ending with a NUL byte'

🔢 Applies elsewhere in the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants