Refactored WP_Debug_Data::debug_data() - get_wp_server() #7283

apermo · 2024-09-03T05:50:11Z

Changed: Moving $info['wp-server'] to own function

Follow up to #7027 as discussed with @dmsnell and @costdev

Summary of the coming steps:

wp-filesystem
wp-constants
wp-database
wp-server (we are here)
wp-media
wp-plugins-active & wp-plugins-inactive
wp-mu-plugins
wp-themes-inactive
wp-parent-theme
wp-active-theme
wp-dropins
wp-paths-sizes
wp-core

Refactoring WP_Debug_Data::debug_data(); into smaller functions for

better maintainability
reduced complexity
preparation for future unit tests
improved extensibility (see trac for details)
added php7.2 style type hints
fixed an error with $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.

github-actions · 2024-09-03T05:50:27Z

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 props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props apermo, dmsnell, costdev, mukesh27, kebbet.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions · 2024-09-03T06:02:18Z

Test using WordPress Playground

The 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

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

apermo · 2024-09-03T05:56:47Z

src/wp-admin/includes/class-wp-debug-data.php

+		}
+
+		// Server time.
+		$date = new DateTime( 'now', new DateTimeZone( 'UTC' ) );


@dmsnell PHPstorm warned me that this line could possibly throw an exception. Since we have a highly controlled set of parameters, I think it's safe to assume we will never get this exception.

Yet PHPstorm wanted me to add a @throws in the PHPdoc, which I ignored for the time being. But what do you suggest what we do about it?

Adding a @throws will just bubble up the problem.

How about

Suggested change

$date = new DateTime( 'now', new DateTimeZone( 'UTC' ) );

try {

$date = new DateTime( 'now', new DateTimeZone( 'UTC' ) );

} catch ( Exception $e ) {

// It should be impossible to reach this statement. But in case let's fail gracefully.

wp_die( $e->get_message() );

}

(Untested and written in GitHub only)

While not impossible (See the Europe/Kyiv change in the "Changes to zone name" section - Which changed for PHP 8.2), since we're using the UTC timezone, the chance of an Exception being thrown for this call is extremely low.

It seems to be fairly safe to consider this a tooling flag that can be ignored.

I don't have strong feelings on this though. It'll probably just come down to preference.

I don't see any notes in the documentation suggesting that UTC was ever not part of the distribution. The constant DateTimeZone::UTC hints at this, and if you ask PHP to get a timezone supported by UTC it returns the string in use here.

php > var_dump( DateTimeZone::listIdentifiers( DateTimeZone::UTC ) ); array(1) { [0]=> string(3) "UTC" }

While sometimes the documentation is wrong, it seems unlikely here, meaning that it may not even be an extremely low chance of throwing here, but not possible.

Do we need a WPCS ignore statement here? If not then I think it's fine as-is. Regardless, as I have written in other comments - this PR is about modularizing a piece of the debug data code, and I recommend against making semantic changes otherwise until that factoring has completed.

It does not trigger WPCS, just triggers the Code analysis from PHPstorm.

So we can keep it as is.

apermo · 2024-09-03T06:04:28Z

src/wp-admin/includes/class-wp-debug-data.php

+		$info['wp-server']     = self::get_wp_server();
 		$info['wp-database']   = self::get_wp_database();
+		$info['wp-constants']  = self::get_wp_constants();
 		$info['wp-filesystem'] = self::get_wp_filesystem();


I reordered this, to be able to get rid of the $info['wp-database'] in lines 199-202

Actually I would like to order the function calls bellow as well. OCD is itching.

This is the correct order of the function calls, and by having them correct, we can completely replace the original $info definition which was the plan of this.
And I'd like it best if we could still swap around the function defintions of get_wp_database() in line 1391 and get_wp_constants() in line 1238 to match this order.

Your thought @dmsnell ?

After discussing in Slack, this all sounds great, and I apologize for reordering these before. The insertion order of the keys in the array determines the order they are displayed in wp-admin, and so we want to preserve the existing order.

@apermo thoughts on this? when I dug into it, I felt like something ad-hoc was missing a bigger point, so I proposed encoding the ordering as a specific semantic instead of relying on how the code is already written.

#7289

@dmsnell updated the PR.

I'll add an inline comment and tag you, you've missed one thing in #7289 that we need to cover in order to match the format for both single and multisite.

src/wp-admin/includes/class-wp-debug-data.php

kebbet

Strings could be more clear, while we are editing this function as a whole.

src/wp-admin/includes/class-wp-debug-data.php

Moved wp-server to a distinct function. Sorted function calls for wp-database

Adaptations for WordPress#7289 Added a condition to unset $info['wp-paths-sizes'] for multisites to match the original state.

apermo · 2024-09-07T06:18:50Z

src/wp-admin/includes/class-wp-debug-data.php

 			'wp-database'         => self::get_wp_database(),
 			'wp-constants'        => self::get_wp_constants(),
 			'wp-filesystem'       => self::get_wp_filesystem(),
 		);

+		// Remove accordion for Directories and Sizes if in Multisite.
+		if ( $is_multisite ) {
+			unset( $info['wp-paths-sizes'] );


@dmsnell See lines 167-173 and 288-348, $info['wp-paths-sizes'] shall only be visible on single sites, so I've added this unset here. I think that was why I originally opted for $info['key'] = self::get_key; instead of your approach.

And I've knowingly used is_multisite() instead of $is_multisite, we will remove $is_multisite = is_multisite();(Line 44) later in the refactoring, since it is likely only helpful in the upcoming function get_wp_core()

this seemms fine. thanks @apermo

I've updated the comment in f576b04

we could eventually filter this array and have self::get_wp_paths_sizes() return null for multi-site deployments.

Changed: replaced $is_multisite with is_multisite() as preparation for a later step.

dmsnell · 2024-09-09T22:27:42Z

src/wp-admin/includes/class-wp-debug-data.php

 			'wp-database'         => self::get_wp_database(),
 			'wp-constants'        => self::get_wp_constants(),
 			'wp-filesystem'       => self::get_wp_filesystem(),
 		);

+		// Remove accordion for Directories and Sizes if in Multisite.


This could probably use some rearranging. I understand that the display of the debug data in wp-admin happens to display sections in a UI like an accordion, but this function doesn't directly couple with that, does it?

Could the comment focus on the purpose of the debug data itself, and not the display?

// Path sizes are only relevant in single-site contexts.

dmsnell · 2024-09-09T22:44:37Z

src/wp-admin/includes/class-wp-debug-data.php

+			$php_version_debug .= ' 64bit';
+		}
+
+		$fields['server_architecture'] = array(


we seem to be assigning keys to $fields here without initializing $fields as a variable.

initialized in e6f03ea

…art4

dmsnell · 2024-09-10T00:03:34Z

Merged in [59002]
19fc9c7

kebbet mentioned this pull request Sep 3, 2024

Move wp-server debug data to separate function. #7284

Closed

apermo commented Sep 3, 2024

View reviewed changes

mukeshpanchal27 reviewed Sep 3, 2024

View reviewed changes

src/wp-admin/includes/class-wp-debug-data.php Show resolved Hide resolved

kebbet reviewed Sep 3, 2024

View reviewed changes

src/wp-admin/includes/class-wp-debug-data.php Show resolved Hide resolved

kebbet reviewed Sep 3, 2024

View reviewed changes

src/wp-admin/includes/class-wp-debug-data.php Show resolved Hide resolved

kebbet reviewed Sep 3, 2024

View reviewed changes

src/wp-admin/includes/class-wp-debug-data.php Show resolved Hide resolved

src/wp-admin/includes/class-wp-debug-data.php Show resolved Hide resolved

apermo and others added 2 commits September 7, 2024 07:52

Refactoring Part 4 - wp-server

99031e5

Moved wp-server to a distinct function. Sorted function calls for wp-database

Refactored WP_Debug_Data::debug_data() Part 4

8952731

Adaptations for WordPress#7289 Added a condition to unset $info['wp-paths-sizes'] for multisites to match the original state.

apermo force-pushed the refactor-wp-debug-data/part4 branch from efc18b9 to 8952731 Compare September 7, 2024 06:07

apermo commented Sep 7, 2024

View reviewed changes

Refactored WP_Debug_Data::debug_data() Part 4

50d05ed

Changed: replaced $is_multisite with is_multisite() as preparation for a later step.

apermo mentioned this pull request Sep 7, 2024

Refactored WP_Debug_Data::debug_data() - get_wp_mu_plugins() #7305

Closed

apermo changed the title ~~Refactored WP_Debug_Data::debug_data() Part 4~~ Refactored WP_Debug_Data::debug_data() - get_wp_server() Sep 7, 2024

dmsnell reviewed Sep 9, 2024

View reviewed changes

Update comment removing pre-filled paths-sizes data.

f576b04

dmsnell reviewed Sep 9, 2024

View reviewed changes

dmsnell added 2 commits September 9, 2024 15:45

Initialize $fields before assignment.

e6f03ea

Merge remote-tracking branch 'upstream' into refactor-wp-debug-data/p…

7c6ab6d

…art4

dmsnell closed this Sep 10, 2024

dmsnell deleted the refactor-wp-debug-data/part4 branch September 10, 2024 00:03

This was referenced Sep 16, 2024

Refactoring - wp-media #7356

Closed

Refactoring - wp-core #7357

Closed

Refactoring - wp-dropins #7418

Closed

This was referenced Sep 27, 2024

Refactoring - wp-paths-sizes #7445

Closed

Refactoring - wp-plugins-active & wp-plugins-inactive #7458

Closed

apermo mentioned this pull request Oct 5, 2024

Refactoring - wp-themes-inactive, wp-parent-theme & wp-active-theme #7507

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored WP_Debug_Data::debug_data() - get_wp_server() #7283

Refactored WP_Debug_Data::debug_data() - get_wp_server() #7283

apermo commented Sep 3, 2024 •

edited

Loading

github-actions bot commented Sep 3, 2024 •

edited

Loading

github-actions bot commented Sep 3, 2024

apermo Sep 3, 2024

costdev Sep 3, 2024

dmsnell Sep 3, 2024

apermo Sep 3, 2024

apermo Sep 3, 2024

dmsnell Sep 3, 2024

dmsnell Sep 4, 2024

apermo Sep 7, 2024

kebbet left a comment

apermo Sep 7, 2024 •

edited

Loading

dmsnell Sep 9, 2024

dmsnell Sep 9, 2024

dmsnell Sep 9, 2024

dmsnell Sep 9, 2024

dmsnell Sep 9, 2024

dmsnell Sep 9, 2024

dmsnell commented Sep 10, 2024

-		$date = new DateTime( 'now', new DateTimeZone( 'UTC' ) );
+		try {
+			$date = new DateTime( 'now', new DateTimeZone( 'UTC' ) );
+		} catch ( Exception $e ) {
+			// It should be impossible to reach this statement. But in case let's fail gracefully.
+			wp_die( $e->get_message() );
+		}

Refactored WP_Debug_Data::debug_data() - get_wp_server() #7283

Refactored WP_Debug_Data::debug_data() - get_wp_server() #7283

Conversation

apermo commented Sep 3, 2024 • edited Loading

github-actions bot commented Sep 3, 2024 • edited Loading

github-actions bot commented Sep 3, 2024

Test using WordPress Playground

Some things to be aware of

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kebbet left a comment

Choose a reason for hiding this comment

apermo Sep 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell commented Sep 10, 2024

apermo commented Sep 3, 2024 •

edited

Loading

github-actions bot commented Sep 3, 2024 •

edited

Loading

apermo Sep 7, 2024 •

edited

Loading