-
Notifications
You must be signed in to change notification settings - Fork 46
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
Conversation
ie. bail on null
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"
}
|
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? |
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. |
I found an issue about this here: WordPress/gutenberg#40557 |
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). |
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. |
There was a problem hiding this 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.
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 anull
value, leading to a fatal error.How?
The main changes include:
array_filter
operates onnull
.Testing Instructions