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

New Widgets Screen does wp_footer action below each Widget #33580

Closed
MadtownLems opened this issue Jul 20, 2021 · 21 comments · Fixed by #36759
Closed

New Widgets Screen does wp_footer action below each Widget #33580

MadtownLems opened this issue Jul 20, 2021 · 21 comments · Fixed by #36759
Assignees
Labels
[Feature] Widgets Screen The block-based screen that replaced widgets.php. Needs Testing Needs further testing to be confirmed. [Status] In Progress Tracking issues with work in progress

Comments

@MadtownLems
Copy link

Description

do_action( 'wp_footer') is apparently called after every widget on the new block-based Widgets screen

Step-by-step reproduction instructions

Use any code that outputs during wp_footer and you'll see that output after each widget on the new widgets screen. (example in code snippet section)

Expected behaviour

Nothing is output on the widgets screen

Actual behaviour

"Why is this being output on the Widgets screen?" is being output after every widget on the widget screen.

Screenshots or screen recording (optional)

2021-07-20 16_09_01-Widgets ‹ Jason D4 Site — WordPress

Code snippet (optional)

add_action( 'wp_footer', 'wp_footer_proof_of_concept' );

function wp_footer_proof_of_concept() {
echo "Why is this being output on the Widgets screen?";
}

WordPress information

  • WordPress version: 5.8
  • Gutenberg version: Not Installed
  • Are all plugins except Gutenberg deactivated? yes (except the proof of concept snippet above)
  • Are you using a default theme (e.g. Twenty Twenty-One)? yes

Device information

  • Device: Desktop
  • Operating system: Windows 10
  • Browser: Chrome 91
@talldan talldan added [Feature] Widgets Screen The block-based screen that replaced widgets.php. Needs Testing Needs further testing to be confirmed. labels Jul 21, 2021
@noisysocks noisysocks added this to the WordPress 5.8.1 milestone Jul 22, 2021
@rvpatel
Copy link

rvpatel commented Jul 27, 2021

This is working fine on development version 5.9-alpha-51272-src with theme (Twenty Twenty-One).

@peterwilsoncc
Copy link
Contributor

Similarly wp_head() is called on each widget's preview too. This may also prove problematic depending on the script/styles enqueued by developers expecting the full front end context.

@peterwilsoncc
Copy link
Contributor

Sharing my notes from trac here too:

I am not sure what the best solution here is.

Some widgets will depend on assets included in wp_head() and wp_footer() for either styling or JavaScript needed for rendering the widget preview. For example the Jetpack's social icons widget alongside several other plugins with widgets in the most popular plugins.

The problem stems from unrelated plugins resuming the front-end for the actions a historically that's been the case and outputting features there as a result, be it analytics code or something that is displayed.

Some possible solutions are:

  • Remove the front-end actions and use new wp_widget_preview_* in their place -- this will require all themes to enqueue default styles especially for the widget previews
  • Leave them in place -- requiring plugins to check is_admin() when enqueueing/adding anything not needed for the widget preview

Both of these seem like bad choices, so I welcome any other suggestions.

As I said, I'm not sure what the best solution is as either seem like a backward compatibility break but it's already been broken with using the formerly front-end only actions in the admin.

@draganescu
Copy link
Contributor

We could simply hide in a div any output?

@MadtownLems
Copy link
Author

Hiding the output in a div only accounts for some of the cases.

I agree that there doesn't appear to be a great solution here, but of all the proposed ones, I'd probably have gone with this one:

"Remove the front-end actions and use new wp_widget_preview_* in their place -- this will require all themes to enqueue default styles especially for the widget previews"

However, the approach feels especially "not great" now that the Widgets screen has already launched without this approach. While I'm slightly surprised to not see more people raising this issue, my assumption is that so many people are using Classic Widgets (or just haven't tried to update their Widgets since 5.8) that it will be popping up more and more for folks over the next several months/year.

@draganescu
Copy link
Contributor

Hiding the output in a div only accounts for some of the cases.

Yes, it was a very raw idea 👍🏻

I'm slightly surprised to not see more people raising this issue

It's also likely that themes don't output content with this action. By the time we reach the closing </body> all content would have been out. Just a guess.

In the trac copy of this issue a patch is evolving which proposes some interesting ideas.

@costdev
Copy link
Contributor

costdev commented Aug 26, 2021

See this comment on Trac for the reason why the possible solution I'd posted that @draganescu referenced above won't be suitable for all cases, as well as a proposal to remove legacy widget previews (instead showing only the form for the legacy widget) until such time as we reach a better solution.

@costdev
Copy link
Contributor

costdev commented Sep 30, 2021

Testing Report

TL;DR

  • Issue still occurs in 5.9-alpha-51876.
  • The patch hides the output from the user.

Env

  • WordPress 5.9-alpha-51876 - Fresh install
  • PHP: 7.4.24
  • Localhost: WSL2 with Ubuntu 18.04
  • OS: Windows 10
  • Browser: Chrome (94.0.4606.61)
  • Theme: Tested in all bundled themes - 2010-2021
  • Plugins: None activated
  • Custom scripts: None

Steps to Test

  1. Open functions.php for the current theme.
  2. Paste the following and save.
add_action( 'wp_footer', 'wp_footer_proof_of_concept' );
function wp_footer_proof_of_concept() {
	echo 'Why is this being output on the Widgets screen?';
}
  1. Go to Appearance > Widgets.
  2. Add a Legacy Widget.

Results (without patch)

  • "Why is this being output on the Widgets screen?" is output on each Legacy Widget preview.

33580-before-patch

Results (with patch applied)

  • "Why is this being output on the Widgets screen?" is hidden from the user.

33580-after-patch

Notes

The patch updates src/wp-includes/blocks/legacy-widget.php with CSS to resolve the issue.

In CI, this file is picked up by webpack:prod, which cleans out the CSS as it doesn't match @wordpress/widgets/src/blocks/legacy-widget/index.php.

This triggers a failure on Ensure version-controlled files are not modified or deleted, so the CSS will also need to be added to @wordpress/widgets/src/blocks/legacy-widget/index.php to pass CI.

@JustinyAhin
Copy link
Member

I've just tested this following the steps here, and I can confirm the patch correctly removes the content outputted by wp_footer().

Testing environment

  • WordPress 5.8.1 - Fresh install
  • PHP: 7.3.5
  • Localhost: Local by Flywheel
  • OS: macOS Monterey 12.0
  • Browser: Firefox Developer Edition (94.0b6)
  • Theme: TT1
  • Plugins: None activated
  • Custom scripts: None

@costdev
Copy link
Contributor

costdev commented Oct 28, 2021

Now that this issue has two test reports that show the patch successfully prevents the content from being visible to the user, how do we progress it to the next/final stage?

@ve3
Copy link

ve3 commented Jan 12, 2022

I'm still see footer everywhere in admin->widgets management & customizer->widgets management pages.

Testing with WordPress 5.9-RC2-52567

If I copy those patch to wp-includes/rest-api/endpoints/class-wp-rest-widget-types-controller.php then it seems to be fixed.
What I have to do with it?

@webd-uk
Copy link

webd-uk commented Jan 4, 2023

Seeing as this patch doesn't look like it's made it into Wordpress core, what is the recommended method for preventing the issue for plugin developers? I have come up with this ... ?

add_action( 'wp_footer', 'wp_footer_proof_of_concept' );

function wp_footer_proof_of_concept() {

if (!(defined('REST_REQUEST') && REST_REQUEST)) {

echo "Why is this being output on the Widgets screen?";

}

}

Please confirm.

Oliver

@draganescu
Copy link
Contributor

@webd-uk searching for the patch in core reveals it at line 105 in wp-includes/blocks/legacy-widget.php. What do you mean by:

Seeing as this patch doesn't look like it's made it into Wordpress core ...

@webd-uk
Copy link

webd-uk commented Jan 4, 2023

Oh. I assumed the patch would prevent Legacy Widgets from firing wp_footer in the editor? wp_footer still fires on Legacy Widgets it would appear ... ?

@MadtownLems
Copy link
Author

The patch certainly doesn't fix the issue for me, either. On 6.1.1, when I use a Legacy Widget, I still get the output from wp_footer rendering on my widgets screen.

@webd-uk
Copy link

webd-uk commented Jan 4, 2023

OK, glad I'm not going mad :)

@ve3
Copy link

ve3 commented Jan 4, 2023

Confirm problem still exists with legacy widget on admin > Appearance > Widgets page.

Tested with WordPress 6.2-alpha-55027

@draganescu
Copy link
Contributor

Yes the patch is a CSS based one, the thing fires but the content is hidden. Definitely not bullet proof. But it did fix a lot of common situations. @MadtownLems could you open a new issue or add here as a comment more details about:

  • your setup
  • your widget
  • the kind of output you see

Thank you!

@webd-uk
Copy link

webd-uk commented Jan 4, 2023

@draganescu OK, but as for my suggestion for plugin developers so that the problem can be sorted from that angle at the same time, do you concur that it's a good solution? If so I'll start rolling it out wherever we use wp_footer hook. Thanks.

@MadtownLems
Copy link
Author

On 6.1.1, wp_footer output is still being shown for Legacy Widgets (fresh 6.1.1 multisite, twenty twenty-one theme), but not for most blocks.

`<?php
/*

  • Plugin Name: wp_footer Widget Test
    */

add_action( 'wp_footer', 'wp_footer_proof_of_concept_53801' );

function wp_footer_proof_of_concept_53801() {
echo "This should really only be on the front-end.";
}`

2023-02-24 08_37_20-Widgets ‹ 2021 — WordPress

@webd-uk
Copy link

webd-uk commented Feb 24, 2023

Yes, this ticket needs to be re-opened I think.

We are actively migrating legacy widgets on client sites to Block Widgets or installing the "Classic Widgets" plugin for widgets that cannot be converted.

For plugins we develop that make use of the wp_footer hook, we are implementing this work-around as and when ...

add_action( 'wp_footer', 'wp_footer_proof_of_concept' );

function wp_footer_proof_of_concept() {

if (!(defined('REST_REQUEST') && REST_REQUEST)) {

echo "Why is this being output on the Widgets screen?";

}

}

Oliver

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Widgets Screen The block-based screen that replaced widgets.php. Needs Testing Needs further testing to be confirmed. [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.