File Block: Remove i18n from save function #43050

petitphp · 2022-08-07T18:02:06Z

What?

Remove calls to __() in file block's save function and move i18n to the PHP render_callback.

Why?

This is an attempt at addressing #43013

As explain in the issue, file block is using __ in its save function which cause the block to become invalid when the locale is changed.

How?

This PR remove the usage of i18n calls in the save function ensuring the content saved in post content is always the same. The translation is handle in the PHP side using the render_callback.

It might not be the best way to address the issue. The aim of this PR is to provide a new markup for new and existing valid blocks to avoid validation error in the future if the locale change. It don't handle the case for existing file block who are already breaking.

Using a tool like the WP_HTML_Walker #42485 could improve the manipulation of the aria attribute, currently this is done through simple regex matching.

Testing Instructions

Create a new post
Upload a pdf to a file block
Publish the post
Change to a different language
Reload the post in the editor
The file block is valid and displayed correctly

swissspidy · 2022-08-08T11:32:34Z

Not using i18n in the save function is good, but just hardcoding to English strings is not good.

petitphp · 2022-08-08T12:47:19Z

Yes I agree. We could just put the filename value in the aria-label when saving the block and let the PHP side generate the complete value. That way we wouldn't have hardcoded or duplicate strings.

…d handle translation in PHP.

petitphp · 2022-08-08T20:11:10Z

I've updated the PR to remove hardcoded English string in the save function and rewrite the PHP to be more strict when looking for the aria-label.

When the block HTML is saved to the post content, the aria-label will only contain the filename. The PHP will parse the block HTML content to look for an object tag with an aria-label and update it with the corresponding translated version.

talldan · 2022-08-09T09:32:20Z

Appreciate the work on this.

It's on my radar to review, but I haven't quite gotten round to reviewing it yet. I'll try to prioritise for tomorrow 👍

dmsnell · 2022-08-09T11:52:30Z

packages/block-library/src/file/index.php

+		},
+		$content
+	);
+


thanks for noticing the work on the WP_HTML_Walker - for reference, here is roughly the code that would replace what we have here…

// Update object's aria-label attribute if present in block HTML. // Match an aria-label attribute from an object tag. $w = new WP_HTML_Walker( $content ); if ( $w->next_tag( 'object' ) ) { $filename = $w->get_attribute( 'aria-label' ) ?: ''; $label = ! empty( $filename ) sprintf( /* translators: %s: filename. */ __( 'Embed of %s.' ), $filename ) : __( 'PDF embed' ); $w->set_attribute( 'aria-label', $label ); return $w; } return $content;

@dmsnell Thanks for the code. Do you think we should we create a follow up issue to update the file block when the work on WP_HTML_Walker is merge ?

sounds like a great idea!

I created the follow-up PR - #60494.

talldan

This works well, for any newly added blocks the invalidation is gone.

Thanks for taking such a thorough approach in this PR.

Comments are all pretty minor, I think the only required change would be to keep outputting the 'PDF embed' from the save function for consistency, but other comments are all optional.

talldan · 2022-08-10T07:40:07Z

packages/block-library/src/file/deprecated.js

-				displayPreview,
-				previewHeight,
-			} = attributes;
+// Version of the file block with the translated aria-label.


It'd be good to reference this PR number in this comment, as the other comment does.

Indeed, I've added the PR number in the comment.

talldan · 2022-08-10T07:52:15Z

packages/block-library/src/file/save.js

-				__( 'Embed of %s.' ),
-				fileName
-		  );
+	const pdfEmbedLabel = RichText.isEmpty( fileName ) ? '' : fileName;


It might be good to keep outputting 'PDF embed' here when there's no fileName. That way the PHP rendering code is treated as a pure progressive enhancement of the JavaScript code.

I've added back the PDF embed string. I've also added some PHP unit test to ensure the PHP rendering is working correctly in the different scenarios.

talldan · 2022-08-10T07:55:33Z

packages/block-library/src/file/index.php

+		$pattern,
+		function ( $matches ) {
+			$filename     = ! empty( $matches['filename'] ) ? $matches['filename'] : '';
+			$has_filename = ! empty( $filename );


If, as per my other comment, 'PDF embed' is output from the JavaScript save function, this would need to change to something like $filename !== 'PDF embed'. Or you could go for ! empty( $attributes['fileName'] ).

I've updated the render function accordingly.

talldan · 2022-08-10T07:57:11Z

packages/block-library/src/file/index.php

+	$content = preg_replace_callback(
+		$pattern,
+		function ( $matches ) {
+			$filename     = ! empty( $matches['filename'] ) ? $matches['filename'] : '';


You could also consider using $attributes['fileName'] to simplify things, though I suppose that would mean introducing a closure.

I originaly though of doing that but I discovered that fileName was not present in the $attributes array in PHP since its sourced from the HTML of the block.

talldan

Apologies for the slow re-review. Thanks for all the hard work on this.

This looks good to me now.

petitphp added 3 commits August 7, 2022 18:14

Remove i18n in file block's save function and update imports

fb2302c

Create new deprecation to handle removing of i18n in save function

b0cdf4a

Handle i18n of aria-label in PHP render callback

a9188fa

petitphp requested a review from ajitbohra as a code owner August 7, 2022 18:02

petitphp changed the title ~~Fix/43013~~ File Block: Remove i18n from save function Aug 7, 2022

petitphp added 2 commits August 8, 2022 17:51

Save file's name in aria-label when outputing html in post content an…

beb5585

…d handle translation in PHP.

Update file block fixtures

09411e5

talldan added [Type] Bug An existing feature does not function as intended [Block] File Affects the File Block labels Aug 9, 2022

dmsnell reviewed Aug 9, 2022

View reviewed changes

talldan reviewed Aug 10, 2022

View reviewed changes

petitphp added 3 commits August 16, 2022 18:02

Add PR number in deprecated version comment

009e15f

Output default string in aria-label for empty filename

3b0456f

Add unit tests for File block render

c9f4951

petitphp requested review from talldan and removed request for ajitbohra August 18, 2022 13:55

talldan approved these changes Sep 12, 2022

View reviewed changes

talldan merged commit b2ad8fa into WordPress:trunk Sep 12, 2022

github-actions bot added this to the Gutenberg 14.1 milestone Sep 12, 2022

t-hamano mentioned this pull request Sep 18, 2022

[New Block] Add post time to read block #43403

Merged

talldan mentioned this pull request Feb 3, 2023

File block with a pdf upload becomes invalid in the editor when changing languages #43013

Closed

petitphp deleted the fix/43013 branch March 31, 2023 13:14

Mamaduka mentioned this pull request Apr 5, 2024

File: Use HTML API to update the PDF preview label #60494

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

File Block: Remove i18n from save function #43050

File Block: Remove i18n from save function #43050

petitphp commented Aug 7, 2022 •

edited

Loading

swissspidy commented Aug 8, 2022

petitphp commented Aug 8, 2022

petitphp commented Aug 8, 2022

talldan commented Aug 9, 2022

dmsnell Aug 9, 2022 •

edited

Loading

petitphp Aug 19, 2022

dmsnell Sep 10, 2022

Mamaduka Apr 5, 2024

talldan left a comment

talldan Aug 10, 2022 •

edited

Loading

petitphp Aug 16, 2022

talldan Aug 10, 2022

petitphp Aug 18, 2022 •

edited

Loading

talldan Aug 10, 2022

petitphp Aug 18, 2022

talldan Aug 10, 2022

petitphp Aug 18, 2022

talldan left a comment

File Block: Remove i18n from save function #43050

File Block: Remove i18n from save function #43050

Conversation

petitphp commented Aug 7, 2022 • edited Loading

What?

Why?

How?

Testing Instructions

swissspidy commented Aug 8, 2022

petitphp commented Aug 8, 2022

petitphp commented Aug 8, 2022

talldan commented Aug 9, 2022

dmsnell Aug 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talldan left a comment

Choose a reason for hiding this comment

talldan Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petitphp Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talldan left a comment

Choose a reason for hiding this comment

petitphp commented Aug 7, 2022 •

edited

Loading

dmsnell Aug 9, 2022 •

edited

Loading

talldan Aug 10, 2022 •

edited

Loading

petitphp Aug 18, 2022 •

edited

Loading