Refactored WP_Debug_Data::debug_data()

[apermo]

Refactored 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

Tested the code multiple times, to ensure that the output is identical to before, as expected only server time did change.

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]

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, mukesh27, dmsnell, costdev.

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

[github-actions]

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.

[dmsnell]

This seems like valuable work; thanks @apermo for putting this together.

At the same time, it's definitely challenging to review, probably for the same reason it might have been challenging to refactor: size and scope.

Since I'm not familiar with this subsystem I would be happy if others (who are) would be able to examine this and evaluate it.

In the case that nobody is available, I might offer breaking this into multiple smaller changes. The raw size of the diff is one thing, but also the renames all combined make it more difficult to follow changes to the control flow. I wonder if we could address each sub-function one at a time where it's easier to isolate the changes and consider each one on its own. For example, there is a lot of code in there working through imagick limits - it would be nice to be able to focus solely on that change and not have to consider it within the scope of all of the others.

This is more work on you, which I find unfortunate, so please take this only as one voice suggesting a way to get it through faster, if that's your goal. This is often how I like to structure my work.

[apermo]

Thanks @dmsnell for the comment. Do understand you correctly that you suggest to basically have 1 PR per added/refactored function/add_action()?
If so, I can surely break that into single commits per new function.

[dmsnell]

Hm. I thought for sure that I responded to this but maybe my connection was bad and the comment was lost. Yes, @apermo I was thinking that one PR for each incremental atomic piece would be helpful simply because it shrinks the surface-area of the review.

For instance, there is a lot of code dedicated to finding limits of Imagick. That could all be isolated in one change that only addresses the Imagick pieces. You are invited to cc me on any splits or follow-ups to this.

[costdev]

Thanks for this work @apermo! I agree with @dmsnell that addressing this in separate PRs would be ideal. I'd recommend starting with one PR for one piece. We can then review this and ensure the approach is supported and is ready for inclusion in Core. From there, the remaining PRs can be created that follow the same approach.

I noticed that some existing methods had return type declarations added to them. I'd suggest that the scope of this work is limited to splitting WP_Debug_Data::debug_data() up to improve maintainability.

[apermo]

@costdev I sure will, i will break this down into multiple commits so that we can do multiples PRs out of it. I will leave this open until I have the first new PR

[apermo]

I have renamed the original branch.

But this PR can now remain closed.

https://github.com/apermo/wordpress-develop/tree/refactor-wp-debug-data-old