-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Refactoring - wp-core #7357
base: trunk
Are you sure you want to change the base?
Refactoring - wp-core #7357
Conversation
Moved wp-core to a distinct function.
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
); | ||
|
||
// Conditionally add debug information for multisite setups. | ||
if ( is_multisite() ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ( is_multisite() ) { | |
if ( $is_multisite ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've checked is_multisite()
and since that is pretty much just a simple check for some constants, I would prefer to completely remove $is_multisite
altogether. But I agree that it makes sense to unify this one way or the other.
Do you agree with my approach?
function is_multisite() {
if ( defined( 'MULTISITE' ) ) {
return MULTISITE;
}
if ( defined( 'SUBDOMAIN_INSTALL' ) || defined( 'VHOST' ) || defined( 'SUNRISE' ) ) {
return true;
}
return false;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mukeshpanchal27 perhaps this was a review typo? your suggested change introduces an error, because this function was split from the main debug_data()
method. there's no $is_multisite
variable defined here.
additionally, if you are recommending a refactor, perhaps it would be good to wait until the debug_data()
method is fully modularized in this sequence of PRs, to first refactor and then change?
Replaced $is_multisite with is_multisite()
'wp-paths-sizes' => array(), | ||
'wp-dropins' => array(), | ||
'wp-active-theme' => array(), | ||
'wp-parent-theme' => array(), | ||
'wp-themes-inactive' => array(), | ||
'wp-themes-inactive' => self::get_wp_themes_inactive(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this meant to be part of this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh no, this was by accident.
'label' => __( 'Inactive Themes' ), | ||
'show_count' => true, | ||
'fields' => array(), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like this other change is still in here
@@ -1024,6 +868,162 @@ public static function debug_data() { | |||
return $info; | |||
} | |||
|
|||
public static function get_wp_core(): array { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind adding the function header/docblock?
Changed: Moving $info['wp-core'] to own function
Follow up to #7027 as discussed with @dmsnell and @costdev
Summary of the coming steps:
Refactoring WP_Debug_Data::debug_data(); into smaller functions for
$parent_theme_auto_update_string
showing the value from$auto_updates_string
(This will be handled only in Step 9)Trac ticket: https://core.trac.wordpress.org/ticket/61648#ticket
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.