Check if theme fonts present before removing

[vcanales]

Fixes: #659

What?

This PR implements a check to verify the presence of theme fonts before attempting to remove them. Additionally, it restructures the handling of deactivated font removal to ensure safety when theme fonts are not present.

Why?

This PR addresses a critical error that occurs when saving changes to a theme after modifying font settings in a child theme, as detailed in GitHub issue #659. The issue resulted from an unhandled scenario where array_filter() was being called on a null value, leading to a fatal error.

How?

The main changes include:

• Adding a check to ensure that theme font families are present before attempting their removal.
• Splitting the deactivation and asset removal process into separate functions for better clarity and error handling.
• Adjusting the logic to bail out early if theme font families are not defined, to prevent errors when array_filter operates on null.

Testing Instructions

1. Create a child theme from TT4.
2. Click the "Save Changes to Theme" button.
3. Verify that no errors occur, and the changes are saved successfully.

[pbking]

When running this I don't expect to find any fonts in my child theme.json. However I'm finding all of the parent theme's fonts (Twenty Twenty Four). I made no changes to the system fonts with the Font Library for this evaluation.

This isn't breakage... testing against /trunk I see the same behavior. But doesn't seem correct and I believe relevant to the changed code. Should we consider addressing that here?

[vcanales]

When running this I don't expect to find any fonts in my child theme.json. However I'm finding all of the parent theme's fonts (Twenty Twenty Four). I made no changes to the system fonts with the Font Library for this evaluation.

This isn't breakage... testing against /trunk I see the same behavior. But doesn't seem correct and I believe relevant to the changed code. Should we consider addressing that here?

I haven't been able to reproduce your issue:

theme.json Child of TT4 after saving changes with no custom fonts:

vcanales@6cd68687a711:/var/www/html/wp-content/themes/tt-1$ cat theme.json
{
	"version": 2,
	"$schema": "https://schemas.wp.org/wp/6.6/theme.json"
}

theme.json on Child of TT4 after activating a custom font — only includes the custom font:

vcanales@6cd68687a711:/var/www/html$ cat wp-content/themes/tt-1/theme.json
{
	"settings": {
		"typography": {
			"fontFamilies": [
				{
					"fontFace": [
						{
							"fontFamily": "\"Abril Fatface\"",
							"fontStyle": "normal",
							"fontWeight": "400",
							"src": [
								"file:./assets/fonts/zOL64pLDlL1D99S8g8PtiKchm-VsjOLhZBY.woff2"
							]
						}
					],
					"fontFamily": "\"Abril Fatface\", system-ui",
					"name": "Abril Fatface",
					"slug": "abril-fatface"
				}
			]
		}
	},
	"version": 2,
	"$schema": "https://schemas.wp.org/wp/6.6/theme.json"
}

trunk just crashes, so I haven't been able to test whether the behavior is or isn't present there.

[vcanales]

I noticed that if I add a custom font to the child theme (using Google Fonts) and Save Changes to Theme, the parent's fonts are gone from the Font Manager; that sounds like a bug with the Fonts Manager itself rather than CBT. It seems to me like parent theme's fonts should be available in the Fonts Manager regardless of whether the child has defined its own custom fonts.

What do you think @matiasbenedetto @pbking?

[t-hamano]

I noticed that if I add a custom font to the child theme (using Google Fonts) and Save Changes to Theme, the parent's fonts are gone from the Font Manager; that sounds like a bug with the Fonts Manager itself rather than CBT.

As I understand it, when a child theme's theme.json has the same properties as the parent theme's theme.json, those properties will be overwritten and not merged.

For example, if you define the following theme.json in a child theme, the parent presets will not exist for duotone presets, gradient presets, color presets, and font presets:

Details

{
	"$schema": "https://schemas.wp.org/trunk/theme.json",
	"version": 2,
	"settings": {
		"color": {
			"duotone": [
				{
					"colors": ["#111111", "#ffffff"],
					"slug": "child",
					"name": "Child Duotone"
				}
			],
			"gradients": [
				{
					"slug": "child",
					"gradient": "linear-gradient(to bottom, #cfcabe 0%, #F9F9F9 100%)",
					"name": "Child Gradient"
				}
			],
			"palette": [
				{
					"color": "#f9f9f9",
					"name": "Child Color",
					"slug": "child"
				}
			]
		},
		"typography": {
			"fontSizes": [
				{
					"name": "Child",
					"size": "0.9rem",
					"slug": "child"
				}
			]
		}
	}
}

I think I've seen discussions about this in the Gutenberg project, but I'm not sure if the current specification can be considered a bug.

[t-hamano]

I think I've seen discussions about this in the Gutenberg project

I found an issue about this here: WordPress/gutenberg#40557

[pbking]

I haven't been able to reproduce your issue:

Weird, I reproduced it again this morning, then completely wiped my environment and tried again and now I'm experiencing what you describe. I'm not sure what was different but this is working as expected now (and trunk is failing as expecting as well).

[pbking]

I noticed that if I add a custom font to the child theme (using Google Fonts) and Save Changes to Theme, the parent's fonts are gone from the Font Manager; that sounds like a bug with the Fonts Manager itself rather than CBT.

As I understand it, when a child theme's theme.json has the same properties as the parent theme's theme.json, those properties will be overwritten and not merged.

Yeah, that's my understanding of the expectations. I remember talking about it a lot during the Blockbase days. I think that replacing, rather than merging, is correct. It gives the child theme more control at the expense of verbosity. I don't see anything amiss with how it's operating.

[pbking]

This fixes the error and seems good to ship.

I agree that cleaning it up as you have started in #661 will be helpful to continue.