Ensure that errors reported from uploading font files are not duplicated #59564
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -2.1 kB (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
| @@ -305,7 +305,14 @@ function FontLibraryProvider( { children } ) { | |||
| __( 'There were some errors installing fonts. %s' ), | |||
| installationErrors.reduce( | |||
| ( errorMessageCollection, error ) => { | |||
| return `${ errorMessageCollection } ${ error.message }`; | |||
| if ( | |||
There was a problem hiding this comment.
I think it would be good to display them as a list if there remain multiple errors after this. I'm not sure if it's native yet or would require lodash uniq.
There was a problem hiding this comment.
I have formatted multiple errors as a list and changed the language for single error types. Updated description to show expected output styling.
getdave
approved these changes
Mar 7, 2024
There was a problem hiding this comment.
I tested this multiple ways via the UI and I couldn't break it.
Principally I modified WP_REST_Font_Faces_Controller::handle_font_file_upload() to simply return a WP_Error object.
I would have liked to have instructions to replicate a scenario where there are multiple different errors so that I could validate the error message printing so if there's a way to do that please let me know.
This is an improvement.
Results
Before
Multiple duplicated messages which are hard to read and contain a hard coded string Fetch error.
Screen.Capture.on.2024-03-07.at.11-07-07.mp4
After (this PR)
Error messages normalized and formatted.
Screen.Capture.on.2024-03-07.at.11-05-34.mp4
I am however noting that a new string is proposed here There was an error installing fonts. %s and this is not allowed during the RC period due to the string freeze.
@pbking are you proposing this is included in WP 6.5 during the RC period? If so could you provide a brief justification for this and it will need to be discussed with the release team? 🙏
Aside: I fixed the lint error in 32c20ab.
| @@ -225,7 +225,7 @@ export async function batchInstallFontFaces( fontFamilyId, fontFacesData ) { | |||
| // Handle network errors or other fetch-related errors | |||
| results.errors.push( { | |||
| data: fontFacesData[ index ], | |||
| message: `Fetch error: ${ result.reason.message }`, | |||
| message: result.reason.message, | |||
There was a problem hiding this comment.
👍 to passing the real error message without the prefix.
Is it worth guarding against empty reason messages and providing a fallback such as Unknown error?
getdave
reviewed
Mar 7, 2024
| '<ul>' + | ||
| installationErrors.reduce( | ||
| ( errorMessageCollection, errorMessage ) => { | ||
| return `${ errorMessageCollection }<li>${ errorMessage }</li>`; |
There was a problem hiding this comment.
I noticed that if I
- upload 3 fonts
- upload the same 3 fonts again
...I see only a single error message A font face matching those settings already exists. despite the fact that all three of the fonts were invalid (already existed).
It's a nit: but I wonder if we could include information in the error about the specific font that caused the error?
getdave
added
[Type] Bug
getdave
requested review from
youknowriad,
matiasbenedetto,
mikachan,
creativecoder,
fabiankaegy and
annezazu
Comment on lines
+286
to
+293
| installationErrors = installationErrors.reduce( | ||
| ( unique, item ) => | ||
| unique.includes( item.message ) | ||
| ? unique | ||
| : [ ...unique, item.message ], | ||
| [] | ||
| ); | ||
|
|
There was a problem hiding this comment.
I think this can be simplified
installationErrors = [ ...new Set( installationErrors.map( item => item.message ) ) ];
It's grammatically incorrect but how do you feel about using the existing string |
There was a problem hiding this comment.
This tests well and looks much neater for single and multiple errors.
For the list, would it be possible to add this to the components styles so the list has bullets:
diff --git a/packages/edit-site/src/components/global-styles/font-library-modal/style.scss b/packages/edit-site/src/components/global-styles/font-library-modal/style.scss
index cfc42aa63a..4f7dce3b8c 100644
--- a/packages/edit-site/src/components/global-styles/font-library-modal/style.scss
+++ b/packages/edit-site/src/components/global-styles/font-library-modal/style.scss
@@ -27,6 +27,11 @@
.components-navigator-screen {
padding: 3px;
}
+
+ .components-notice ul {
+ margin-left: $grid-unit-15;
+ list-style: disc;
+ }
}
.font-library-modal__tabpanel-layout {The code makes sense to me but I'll let one of the regulars approve it as others have much stronger knowledge of the JavaScript code base.
For testing multiple errors, I changed the return value in the rest controller to:
return new WP_Error( 'rest_font_fetch_errors', __( 'Demonstration of multiple fetch errors ' . rand( 0, 15 ), 'gutenberg' ) );
youknowriad
reviewed
Mar 8, 2024
| }, | ||
| '' | ||
| ) | ||
| '<ul>' + |
There was a problem hiding this comment.
Isn't it a bit weird to use HTML in error messages?
There was a problem hiding this comment.
If we don't then the multiple message appear on a single line and thus this PR wouldn't solve the problem.
Is there a better way to display multiple error messages whilst controlling the rendered result?
There was a problem hiding this comment.
Where is the rendering happening? If we're retrieving this value elsewhere, and we absolutely need to throw an error here, we can create a custom Error class
class FontErrors extends Error {
constructor(serverErrors, ...params) {
super(...params);
this.serverErrors = serverErrors;
}
}
There was a problem hiding this comment.
Simpler:
async function installFonts() { throw { errors: installationErrors } };
installFonts().catch( ( ( { errors } ) => console.log( errors ) );
There was a problem hiding this comment.
I should have checked this more carefully and you are of course correct.
We should format the errors are the point of rendering and not in the place where they are processed.
Ideally the component which renders the errors should receive the raw data and then choose how it renders rather than printing HTML.
There was a problem hiding this comment.
@pbking I moved the error formatting in 18c1af7.
So the errors object that is returned will contain the same error message and potentially the dedicated messages about the specific errors.
I did consider returning AggregateError but that looks to be too new to rely on at this time.
Please feel free to tidy and improve my code or reject it.
getdave
added a commit
that referenced
this pull request
Mar 11, 2024
…ted (#59564) Co-authored-by: pbking <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: peterwilsoncc <[email protected]> Co-authored-by: matiasbenedetto <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: mikachan <[email protected]>
|
I just cherry-picked this PR to the pick/wp-65-rc-2 branch to get it included in the next release: 77aff9d |
getdave
removed
the
Backport to WP 6.7 Beta/RC
getdave
added
the
Backported to WP Core
getdave
added a commit
that referenced
this pull request
Mar 12, 2024
…ted (#59564) Co-authored-by: pbking <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: peterwilsoncc <[email protected]> Co-authored-by: matiasbenedetto <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: mikachan <[email protected]>
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Mar 27, 2024
…ted (WordPress#59564) Co-authored-by: pbking <[email protected]> Co-authored-by: getdave <[email protected]> Co-authored-by: peterwilsoncc <[email protected]> Co-authored-by: matiasbenedetto <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: mikachan <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Ensure that errors reported from uploading font files are not duplicated
Why?
Fixes #59484
How?
Only concat strings that don't exist in the error message string.
Testing Instructions
Using the 'Upload' tab of the Font Library upload multiple fonts at once.
After the fonts are successfully uploaded upload the SAME fonts again.
Note that the message "A font face matching those settings already exists" is only shown a single time.
NOTE: If there are multiple KINDS of messages then those are shown as a list. (I'm... not sure how to replicate this however, in the below example I just removed the reducing logic and am showing the same message multiple times... which wouldn't actually happen now...)