Legacy widget's preview functionality is broken when the page is moved #34011

anton-vlasenko · 2021-08-11T12:57:17Z

Description

Legacy Widget block has a preview function, however, it uses a relative URL.
iframe cannot load the widgets.php file when the code is no longer running from the /wp-admin/ URL.
Fixes #30049

How has this been tested?

Go to Appearance -> Widgets page.
Click on the "plus sign" button at the very bottom of the page.
Add a Legacy Widget block.
Select Meta widget from the dropdown.
Click somewhere on the background of the page (not on the Meta widget itself). You should see a preview of the Meta widget now.
Open your browser's developer tools and find an <iframe /> element with the wp-block-legacy-widget__edit-preview-iframe class. Check its src attribute.

Expected result:

The src attribute of the <iframe /> should contain an absolute URL.
Please see the screenshots section for reference.

Screenshots

Types of changes

Bug fix.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
I've tested my changes with keyboard and screen readers.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

…t and get WP Admin URL. This is needed because we have to have the ability to get the actual WordPress admin URL for some blocks (e.g. legacy widgets block).

…e url for widgets.php iframe. We have to use absolute URLs because in some cases (frontenberg) relative URL is not enough and we have to explitly specifiy path to load widgets.php from.

…/url

We need this to know absolute URL of the WP's admin panel. Some components need that value.

It fixes the issue described in #30049.

github-actions · 2021-08-11T12:57:47Z

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @anton-vlasenko! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

kevin940726

This generally looks pretty good! I'm not sure whether it's the best place to add the editor settings though, could get some 👀 from PHP folks too.

lib/editor-settings.php

packages/widgets/src/blocks/legacy-widget/edit/preview.js

2. Rename wpAbsoluteAdminUrl to adminUrl. IMO This name is shorter and better.

2. Don't assume adminUrl is defined. If it's not defined, return empty string instead.

hellofromtonya · 2021-08-12T15:35:07Z

lib/editor-settings.php

@@ -28,6 +28,9 @@ function gutenberg_extend_post_editor_settings( $settings ) {
 		$settings['defaultTemplatePartAreas'] = gutenberg_get_allowed_template_part_areas();
 	}

+	// Some blocks have to use absolute url.
+	$settings['adminUrl'] = is_callable( 'admin_url' ) ? admin_url() : '';


function_exists is the default pattern for determining if a function is in memory for use

I'm wondering: Should a REST endpoint be used instead of the backend admin URL?

Gutenberg is using the REST API and is not loading the full backend functionality. I'm wondering if using admin_url() here, assuming it's loaded into memory at this point, might expose unintentional leaks.

Hmm not sure. Raising the point for consideration. cc @azaozz @TimothyBJacobs

I don't mind using function_exists.
Fixed in 198191e

I'm wondering: Should a REST endpoint be used instead of the backend admin URL?

+1 That's a very good question.

There is a ticket about creating a REST API endpoint for this whole thing, I'm not sure what the expected timing on that is, WordPress/wordpress-develop#1508. CC: @adamziel.

Should we wait for this simple fix to land after we have that endpoint?

This fix will be ported into core, 100% that function will be there. It is not a feature of the plugin, it is a feature of how core initializes the editors. We've used this in e.g. modify_welcome_panel to modify links in WP Admin with no issues.

So I think we can safely use it as it does not load more resources, it does not tie Gutenberg to core more, and it's a quick fix for other problems of the future: having the admin url available as a setting,

I was away for last two weeks but I'm back now. I will fix the test failures on WordPress/wordpress-develop#1508 and then we'll only be an approval away from merging.

@draganescu

Should we wait for this simple fix to land after we have that endpoint?

Maybe I'm not getting it, but I'd not merge this ticket now because, as @hellofromtonya mentioned, it will expose the backend admin URL. Instead, I'd wait until @adamziel's PR is merged. Then we can implement the fix using a REST endpoint.

I fixed the tests as promised, all the reviews are welcome :D

…sts is the recommended way to do it.

draganescu · 2021-08-12T17:44:31Z

lib/editor-settings.php

@@ -28,6 +28,9 @@ function gutenberg_extend_post_editor_settings( $settings ) {
 		$settings['defaultTemplatePartAreas'] = gutenberg_get_allowed_template_part_areas();
 	}

+	// Some blocks have to use absolute url.
+	$settings['adminUrl'] = function_exists( 'admin_url' ) ? admin_url() : '';


Should we better create our own filter? This function is a shim that exists for WordPress 5.7 and may get removed so we should not tie into it. I think it's better to define something like gutenberg_add_admin_url_to_editor_settings and run that through add_filter just like gutenberg_extend_post_editor_settings.

Don't think there's a chance admin_url() will be missing. It is in wp-includes/link-template.php which is included before the REST API and plugins.

draganescu · 2021-08-12T17:52:36Z

packages/widgets/src/blocks/legacy-widget/edit/preview.js

+		return select( blockEditorStore ).getSettings();
+	}, [] );
+
+	const widgetPreviewUrl = ( adminUrl ?? '' ) + 'widgets.php';


Will we ever not have adminUrl?

draganescu · 2021-08-12T17:53:21Z

cc @noisysocks do you think this fix can land like this in 5.8.1 and maybe later we'll just have a nice REST endpoint for rendering legacy widgets?

packages/widgets/src/blocks/legacy-widget/edit/preview.js

noisysocks · 2021-08-19T03:21:35Z

cc @noisysocks do you think this fix can land like this in 5.8.1 and maybe later we'll just have a nice REST endpoint for rendering legacy widgets?

I don't think the issue is that urgent. Happy to wait for a proper fix.

…I endpoint.

anton-vlasenko · 2021-08-19T21:24:08Z

This is the new PR with the proper fix that leverages the new REST API endpoint.
Let's close this PR when that new PR is merged (just saying).

anton-vlasenko · 2021-08-25T15:09:35Z

Closed in favour of #34184

anton-vlasenko added 8 commits August 10, 2021 18:34

1. Add new getWPAdminURL and setWPAdminURL functions to be able to se…

9829ad8

…t and get WP Admin URL. This is needed because we have to have the ability to get the actual WordPress admin URL for some blocks (e.g. legacy widgets block).

Use getWPAdminURL instead of addQueryArgs to generate correct absolut…

dac4f40

…e url for widgets.php iframe. We have to use absolute URLs because in some cases (frontenberg) relative URL is not enough and we have to explitly specifiy path to load widgets.php from.

Use getWPAdminURL from @wordpress/url becase we removed it from utils…

fdeac24

…/url

Add wpAbsoluteAdminUrl to the block editor settings.

bf2401e

We need this to know absolute URL of the WP's admin panel. Some components need that value.

Revert the changes as we are not going to implement it like that.

54238b3

Use an absolute URL for widget preview.

7af12cd

It fixes the issue described in #30049.

Fix code style (automatic).

ed4d56f

Rename the variable.

f11a7e7

anton-vlasenko requested review from spacedmonkey and TimothyBJacobs as code owners August 11, 2021 12:57

github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Aug 11, 2021

Fix code style.

42a3aa0

anton-vlasenko changed the title ~~Legacy Widget block is not able to render preview in some cases~~ Legacy widget's preview functionality is broken when the page is moved Aug 11, 2021

anton-vlasenko requested a review from kevin940726 August 11, 2021 13:31

Remove redundant line at the end of the file.

62a956a

anton-vlasenko added the [Type] Bug An existing feature does not function as intended label Aug 11, 2021

anton-vlasenko added this to the WordPress 5.8.1 milestone Aug 11, 2021

kevin940726 reviewed Aug 12, 2021

View reviewed changes

lib/editor-settings.php Outdated Show resolved Hide resolved

packages/widgets/src/blocks/legacy-widget/edit/preview.js Outdated Show resolved Hide resolved

packages/widgets/src/blocks/legacy-widget/edit/preview.js Outdated Show resolved Hide resolved

anton-vlasenko added 3 commits August 12, 2021 12:11

1. Check if the admin_url function exists before calling it.

b30ec6b

2. Rename wpAbsoluteAdminUrl to adminUrl. IMO This name is shorter and better.

1. Use destruct assignment to get the value of admin URL.

36370d9

2. Don't assume adminUrl is defined. If it's not defined, return empty string instead.

1. Use destruct assignment to get the value of admin URL.

b6f24f3

2. Don't assume adminUrl is defined. If it's not defined, return empty string instead.

anton-vlasenko requested a review from kevin940726 August 12, 2021 11:23

Fix code style.

288266d

hellofromtonya reviewed Aug 12, 2021

View reviewed changes

Use function_exists instead of is_callable because using function_exi…

198191e

…sts is the recommended way to do it.

draganescu reviewed Aug 12, 2021

View reviewed changes

youknowriad reviewed Aug 12, 2021

View reviewed changes

packages/widgets/src/blocks/legacy-widget/edit/preview.js Outdated Show resolved Hide resolved

Use context to get adminUrl value.

bb21211

anton-vlasenko requested review from adamziel, noisysocks, talldan and tellthemachines as code owners August 16, 2021 09:42

Use context to get adminUrl value (customizer page).

2e4e645

Get rid of the WidgetsSettings and load preview using the new REST AP…

5ac6a14

…I endpoint.

anton-vlasenko closed this Aug 25, 2021

snyk-bot mentioned this pull request Sep 11, 2022

[Snyk] Upgrade react-native from 0.66.2 to 0.69.4 captainbarber99/gutenberg#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Legacy widget's preview functionality is broken when the page is moved #34011

Legacy widget's preview functionality is broken when the page is moved #34011

anton-vlasenko commented Aug 11, 2021 •

edited

Loading

github-actions bot commented Aug 11, 2021

kevin940726 left a comment

hellofromtonya Aug 12, 2021

anton-vlasenko Aug 12, 2021 •

edited

Loading

TimothyBJacobs Aug 12, 2021

draganescu Aug 12, 2021

adamziel Aug 16, 2021

anton-vlasenko Aug 16, 2021 •

edited

Loading

adamziel Aug 17, 2021

draganescu Aug 12, 2021

azaozz Aug 12, 2021 •

edited

Loading

draganescu Aug 12, 2021

draganescu commented Aug 12, 2021

noisysocks commented Aug 19, 2021

anton-vlasenko commented Aug 19, 2021 •

edited

Loading

anton-vlasenko commented Aug 25, 2021

Legacy widget's preview functionality is broken when the page is moved #34011

Legacy widget's preview functionality is broken when the page is moved #34011

Conversation

anton-vlasenko commented Aug 11, 2021 • edited Loading

Description

How has this been tested?

Expected result:

Screenshots

Types of changes

Checklist:

github-actions bot commented Aug 11, 2021

kevin940726 left a comment

Choose a reason for hiding this comment

hellofromtonya Aug 12, 2021

Choose a reason for hiding this comment

anton-vlasenko Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

TimothyBJacobs Aug 12, 2021

Choose a reason for hiding this comment

draganescu Aug 12, 2021

Choose a reason for hiding this comment

adamziel Aug 16, 2021

Choose a reason for hiding this comment

anton-vlasenko Aug 16, 2021 • edited Loading

Choose a reason for hiding this comment

adamziel Aug 17, 2021

Choose a reason for hiding this comment

draganescu Aug 12, 2021

Choose a reason for hiding this comment

azaozz Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

draganescu Aug 12, 2021

Choose a reason for hiding this comment

draganescu commented Aug 12, 2021

noisysocks commented Aug 19, 2021

anton-vlasenko commented Aug 19, 2021 • edited Loading

anton-vlasenko commented Aug 25, 2021

anton-vlasenko commented Aug 11, 2021 •

edited

Loading

anton-vlasenko Aug 12, 2021 •

edited

Loading

anton-vlasenko Aug 16, 2021 •

edited

Loading

azaozz Aug 12, 2021 •

edited

Loading

anton-vlasenko commented Aug 19, 2021 •

edited

Loading