Skip to content
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

Check if theme fonts present before removing #660

Merged
merged 5 commits into from
Jun 4, 2024

Conversation

vcanales
Copy link
Member

@vcanales vcanales commented May 31, 2024

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.

@vcanales vcanales requested a review from t-hamano May 31, 2024 09:00
@vcanales vcanales marked this pull request as draft May 31, 2024 10:18
@vcanales vcanales marked this pull request as ready for review May 31, 2024 10:23
@pbking
Copy link
Contributor

pbking commented May 31, 2024

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.

image

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

t-hamano commented Jun 1, 2024

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
Copy link
Contributor

t-hamano commented Jun 1, 2024

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

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

@pbking
Copy link
Contributor

pbking commented Jun 3, 2024

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
Copy link
Contributor

pbking commented Jun 3, 2024

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.

Copy link
Contributor

@pbking pbking left a comment

Choose a reason for hiding this comment

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

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.

@vcanales vcanales merged commit d6f7432 into trunk Jun 4, 2024
2 checks passed
@vcanales vcanales deleted the fix/crash-on-child-theme-font-use branch June 4, 2024 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants