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

Font Library: Fonts still listed as installed after they are manually deleted via FTP or by other means #58375

Open
afercia opened this issue Jan 29, 2024 · 14 comments
Labels
[Feature] Font Library Needs Decision Needs a decision to be actionable or relevant [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented Jan 29, 2024

Description

Users can delete, rename, move, files via FTP. While one could argue why users would ever want to delete font files via FTP the answer is that the will because it is possible to do it. See the Murphy's law in its original formulation:

If there are two or more ways to do something and one of those results in a catastrophe, then someone will do it that way.

Files may get corrupted as well because of hosting hiccups or because of any other reason.

The Font Library doesn't appear to handle these cases and for example still lists fonts as 'installed' even though the font files doesn't exist any longer. Basically, the user interface will provide users with information that is incorrect and misleading.

I'd think that, like other cases where WordPress functionalities depend on files stored on the filesystem that aren't part of a default installation e.g. patterns or plugins, the functionality should check whether those files exist. Of course, I do realize checking if file exists in the filesystem comes with performance concerns but there should be some check in place.

Ideally, like for plugins, the font should be deactivated and unlisted. For example, when plugin files get deleted, the plugin gets deactivated and a notice appears to inform users:

The plugin {plugin name} has been deactivated due to an error: Plugin file does not exist.

Step-by-step reproduction instructions

  • Go to Site editor > Styles > Typography > Manage fonts
  • In the Fonts modal dialog, go to the Google fonts and install a font e.g. 'Abel'.
  • Close the Fonts modal dialog.
  • If you didn't install any new font previously, a new 'fonts' directory is created in wp-content/fonts/.
  • Locate the fonts directory and delete any font files.
  • Refresh the editor page.
  • Open the Fonts modal dialog again.
  • Observe the font 'Abel' is still listed in the fonts Library.
  • Observe your browser console reports an error 404.
  • In my testing, I also get a random Uncaught DOMExceptionerror apparently initiated by loadFontFaceInBrowser. Screenshot:

Screenshot 2024-01-29 at 10 45 36

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@afercia afercia added [Type] Bug An existing feature does not function as intended [Feature] Font Library labels Jan 29, 2024
@jordesign jordesign added the Needs Testing Needs further testing to be confirmed. label Jan 29, 2024
@colorful-tones
Copy link
Member

colorful-tones commented Jan 30, 2024

I feel like there is potential overlap with #58411. If a user is looking to delete a font amongst fonts and they all have random string names then they’ll likely end up deleting the wrong one.

@matiasbenedetto
Copy link
Contributor

The Font Library doesn't appear to handle these cases and for example still lists fonts as 'installed' even though the font files doesn't exist any longer. Basically, the user interface will provide users with information that is incorrect and misleading.

The installation consists of 2 parts: writing the file assets and storing the font family definition in the database. If you remove the font assets manually, you will end up with database records pointing to files no longer available on the file system. I think this follows the media library pattern. For example, if you upload an image file using the media library and then remove the image files created on disk, you will end up with empty images pointing to no longer available assets. Do you think that the font library should behave differently that the media library on that aspect?

image

@matiasbenedetto matiasbenedetto added the Needs Decision Needs a decision to be actionable or relevant label Jan 30, 2024
@colorful-tones
Copy link
Member

Do you think that the font library should behave differently that the media library on that aspect?

I'm glad you provided additional context and clarification, @matiasbenedetto 👍 I forgot that the Media Library behaves like this. It is undoubtedly critical to compare and consider existing components for UI/UX and even implementation.

However, it would alleviate concern from everyday builders if they encountered such a scenario that there would be any indication that the font files are gone. They can no longer utilize the font in their theme. In my current testing of the Font Library - if I delete a font in wp-content/fonts and then visit the Font Library, there is no indication that anything changed. At least with media, you see an empty square (from the screenshot above). Could we consider some messaging and interface indications that the font linkage has been broken? That seems like an ideal usability enhancement. 🤔
Screenshot 2024-01-30 at 4 05 30 PM

Screenshot 2024-01-30 at 3 54 11 PM
Screenshot 2024-01-30 at 3 54 20 PM

@matiasbenedetto
Copy link
Contributor

if I delete a font in wp-content/fonts and then visit the Font Library, there is no indication that anything changed. At least with media, you see an empty square (from the screenshot above). Could we consider some messaging and interface indications that the font linkage has been broken? That seems like an ideal usability enhancement.

It's a fair point to consider. I would like to get @jasmussen's input about the interface.

@jasmussen
Copy link
Contributor

This issue is not unique to the font library, it's in fact identical to the media library. If you delete files off your server, it's hard for WordPress to be aware of what happens. Here's what happens when you do that to an image in your media library. First, you have this placeholder:

before, media library placeholder image has a thumbnail

before, media library placeholder image opened in modal shows up

I then proceeded to delete "placeholder.png" from wp-content/uploads. That's the first image in the media library screenshots above, here's what happens:

after, the db item still exists, the image is just broken

after, the same db item exists, the image in the modal is also broken

So in the above two screenshots shown after clearing cache and reloading the media library, the database item still exists.

I'd agree it would be nice if, across the WordPress media library and font library, to show some sort of error message indicating the file has been deleted from the server. There are probably technical questions to handle, but given the verbosity and complexity of the steps involved in getting yourself into this state, and that this behavior has been present in WordPress for perhaps a decade, I'd file this in the Enhancement category, and welcome PRs!

@afercia
Copy link
Contributor Author

afercia commented Feb 1, 2024

Not sure comparing with the media library is a good argument, no one said the media library behavior is ideal 🙂

it would alleviate concern from everyday builders if they encountered such a scenario that there would be any indication that the font files are gone.

there is no indication that anything changed. At least with media, you see an empty square

This's a fair point.

For fonts, it would be hard to understand, for example if a rendered font actually uses the 'real' weight 700 or if the browser resorts to the closer font-weight or it uses its own 'bold'.

Copy link

github-actions bot commented Mar 3, 2024

Hi,
This issue has gone 30 days without any activity. This means it is time for a check-in to make sure it is still relevant. If you are still experiencing this issue with the latest versions, you can help the project by responding to confirm the problem and by providing any updated reproduction steps.
Thanks for helping out.

@github-actions github-actions bot added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Mar 3, 2024
@afercia
Copy link
Contributor Author

afercia commented Mar 4, 2024

Thank you github-actions bot, this issue can still be reproduced following the steps in the description.

@github-actions github-actions bot removed the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Mar 5, 2024
@skorasaurus skorasaurus removed the Needs Testing Needs further testing to be confirmed. label Mar 14, 2024
@peterwilsoncc
Copy link
Contributor

I don't think there is anything WordPress can do about this.

As mentioned above, it's no different to the media library. For files with an associated post object in the database then the post object should be deleted in order to delete the files.

It's similar to deleting assets from a theme without modifying the references in the other files. It's going to cause problems.

I think this can be closed as unplanned.

@afercia
Copy link
Contributor Author

afercia commented Apr 12, 2024

I don't think there is anything WordPress can do about this.

Not my specific area of expertise but to my understanding there are ways to prevent this issue. WordPress already checks files on the filesystem, for example it does that for the theme files (even though for different purposes). It also checks missing plugin files, deactivates the plugin and prints out a notice 'The plugin {plugin name} has been deactivated due to an error: Plugin file does not exist' when plugin files are missing. WordPress also reads the plugins headers and does many other things to read files on the filesystem. Of course these checks have a cost in terms of performance but they can be mitigated.

it's no different to the media library

There are differences, for example:

  • A missing media file e.g. an image will be immediately noticed by users because the browsers clearly shows the image is missing.
  • A missing font file may not be noticed at all. For example if a 'bold' variant of a font is missing, the browser will fallback to the normal weight variant and it will apply its own 'bold' rendering. The visual difference may be barely noticeable.

@peterwilsoncc
Copy link
Contributor

A missing font file may not be noticed at all. For example if a 'bold' variant of a font is missing, the browser will fallback to the normal weight variant and it will apply its own 'bold' rendering. The visual difference may be barely noticeable.

I think this is another reason to close the ticket as unplanned.

As the browser gracefully handles missing variants (some cases better than others) then removing a defined font variation because a file is missing creates more of a problem than it solves.

Deleting files from the uploads folder without deleting the associated post object is out of warranty behavior. The post objects are intended to be deleted via WordPress (be that the UI, WP-CLI, or an API request).

@afercia
Copy link
Contributor Author

afercia commented Apr 15, 2024

Deleting files from the uploads folder without deleting the associated post object is out of warranty behavior.

I'd argue that would appply to theme files as well, however we do safeguard against missing theme files.

@peterwilsoncc
Copy link
Contributor

I'm not seeing safeguards for asset files, are you able to link to an example?

I know locate_template() and similar do file_exists() checks but that's more to do with supporting child themes and the template hierarchy than accounting for the deletion of assets.

Deleting fonts, images and stylesheets from various themes results in 404 errors in my testing.

@afercia
Copy link
Contributor Author

afercia commented Apr 16, 2024

Tnanks for pointing out the 404 issue. I think it's important to prevent those potential 404 when a font file is missing. Potentially, users may not ever notice a font file is missing and never notice the related 404s. I think WordPress should prevent them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Font Library Needs Decision Needs a decision to be actionable or relevant [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

7 participants