Skip to content
This repository has been archived by the owner on Mar 9, 2024. It is now read-only.

Add block compatibility debug info to Site Health #169

Merged
merged 6 commits into from
Oct 5, 2023

Conversation

xxsimoxx
Copy link
Member

@xxsimoxx xxsimoxx commented Aug 4, 2023

Description

This PR adds debug informations to Site Health when Blocks Compatibility is in "Troubleshooting" mode.

How has this been tested?

Local testing.

Screenshots

image image

Types of changes

  • New feature

Copy link
Member

@viktorix viktorix left a comment

Choose a reason for hiding this comment

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

Minor text changes.

src/wp-includes/classicpress/class-cp-debug-compat.php Outdated Show resolved Hide resolved
src/wp-includes/classicpress/class-cp-debug-compat.php Outdated Show resolved Hide resolved
@mattyrob
Copy link
Collaborator

mattyrob commented Sep 5, 2023

I see this is now loading in src/wp-settings.php - this means it will load on every single call to a site doesn't it?

The src/wp-admin/includes/class-wp-site-health.php file is only included in settings if needed by CRON tests so perhaps we need to do the same and then include the new class for the tests in src/wp-admin/site-health.php.

Does that make sense?

@xxsimoxx
Copy link
Member Author

xxsimoxx commented Sep 5, 2023

Yes, the class is called on every site load.
We need to catch every polyfill call, that can happen even just in a 404 page.

The code that renders the test in Site Health is hooked to site_status_tests that's only executed by WP_Site_Health::get_tests(). Same happens for debug_information that is executed in WP_Debug_Data::debug_data().

I think this is correct, but please @mattyrob tell me if you agree with this.

I can split the code in two classes. This means less code (not so much as I must duplicate some functions between the class) loaded (but not executed). But it's more confusing to me.

@mattyrob
Copy link
Collaborator

mattyrob commented Sep 5, 2023

Yes, the class is called on every site load. We need to catch every polyfill call, that can happen even just in a 404 page.

The code that renders the test in Site Health is hooked to site_status_tests that's only executed by WP_Site_Health::get_tests(). Same happens for debug_information that is executed in WP_Debug_Data::debug_data().

I think this is correct, but please @mattyrob tell me if you agree with this.

I can split the code in two classes. This means less code (not so much as I must duplicate some functions between the class) loaded (but not executed). But it's more confusing to me.

I'm trying to figure out fully how this works and integrates with the existing compatibility class. Am I right that the current code with two classes is running a trace twice to detect block based code? If so, could we not just access the database values from the wp-includes/classicpress/class-wp-compat.php class - namely the plugins_using_blocks database value and maybe extend the theme_using_blocks option rather than duplicating the code base? Am I missing something fundamental in the way this all works?

@xxsimoxx
Copy link
Member Author

xxsimoxx commented Sep 6, 2023

debug_backtrace (that is the "heavy" part) is called just once by WP_Compat::using_block_function.
In WP_Compat::using_block_function the hook using_block_function is defined and CP_Debug_Compat::log is hooked there and gets the trace result.

Yes, we have two options defined for almost the same thing, but the one defined in CP_Debug_Compat contains a more detailed log and the two in WP_Compat are lighter and formatted to be used quickly in plugins and themes pages.

My idea is to allow people to disable Site Health (as I do on many sites).

Ideally people should put Blocks Compatibility in troubleshooting when setting installing plugins and themes, and then put it on or off depending on installed plugins and theme, so no overhead when not in troubleshooting.

@mattyrob
Copy link
Collaborator

mattyrob commented Sep 6, 2023

I think I have a better under standing now.

I've noticed (I think) and issue on the current code if the Settings option (blocks_compatibility_level) is changed such that the setting is 0 or Off - I then get a "There has been a critical error on this website. Please check your site admin email inbox for instructions."

So, I think this might be linked to the two classes and the use of the 'update_option_blocks_compatibility_level' hook.

Ignore the above, the issue was I had a test plugin using a block function that was polyfilled, and when I disabled comparability I got an error, deactivating that test plugin resolved my critical error issue. Point still stands about the update_option_blocks_compatibility_level' hook potentially being in a 'settings' class with the debug and site health classes conditionally loaded from there.

If that was in a separate class, and that was always loaded in src/wp-settings I think we could conditionally load both the existing src/wp-includes/classicpress/class-wp-compat.php class and the new src/wp-includes/classicpress/class-cp-debug-compat.php class in this PR.

That would reduce the code overhead being loaded if the Compatibility option was set to Off, and if maybe also if Site Health was disabled for example. It may also address the issue I've run into if that can be replicated.

@xxsimoxx
Copy link
Member Author

xxsimoxx commented Sep 7, 2023

Now src/wp-includes/classicpress/class-wp-compat.php is loaded conditionally in wp-settings.php.
src/wp-includes/classicpress/class-cp-debug-compat.php is loaded conditionally in class-wp-compat.php.

In this way (maybe) a plugin can require src/wp-includes/classicpress/class-wp-compat.php even if Block Compatibility is turned off.

There is no way to check if Site Health is disabled, as you can turn it off by removing the menu or by clean the tests array, and that is done after this code load.

@mattyrob
Copy link
Collaborator

mattyrob commented Sep 7, 2023

Recent changes look good - do they impact on the hook to the option purge hooked function? Doe that still work purging the saved functions if compatibility mode setting is changed to Off?

@xxsimoxx
Copy link
Member Author

xxsimoxx commented Sep 7, 2023

No, the hook is not affected. It's needed only when switching from debug to something else, and in that case the hook is in place.

@viktorix
Copy link
Member

viktorix commented Sep 7, 2023

I haven't had the chance to test it yet. I'm curious if this would affects results of wp doctor and wp profile commands.

@xxsimoxx
Copy link
Member Author

xxsimoxx commented Oct 5, 2023

Don't know if it is what Viktor asked about but for wp profile:

wp profile stage --fields=stage,time,cache_ratio

add/blockcompat                        develop
+------------+---------+-------------+ +------------+---------+-------------+
| stage      | time    | cache_ratio | | stage      | time    | cache_ratio |
+------------+---------+-------------+ +------------+---------+-------------+
| bootstrap  | 0.1025s | 97.87%      | | bootstrap  | 0.1014s | 97.66%      |
| main_query | 0.0082s | 94.67%      | | main_query | 0.0083s | 94.67%      |
| template   | 0.0223s | 96.83%      | | template   | 0.0202s | 96.83%      |
+------------+---------+-------------+ +------------+---------+-------------+
| total (3)  | 0.1329s | 96.46%      | | total (3)  | 0.13s   | 96.39%      |
+------------+---------+-------------+ +------------+---------+-------------+

About wp doctor... see #235 .

@mattyrob mattyrob merged commit 8bda8d4 into ClassicPress:develop Oct 5, 2023
@xxsimoxx xxsimoxx deleted the add/blockcompat branch October 5, 2023 15:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants