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

Jump to bottom

Hook timing of edit_form_advanced #10929

Closed
moorscode opened this issue Oct 23, 2018 · 11 comments
Closed

Hook timing of edit_form_advanced #10929

moorscode opened this issue Oct 23, 2018 · 11 comments
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability Needs Technical Feedback Needs testing from a developer perspective. [Type] Bug An existing feature does not function as intended
Milestone
WordPress 5.0 RC

Comments

@moorscode
Copy link
Contributor

Describe the bug

In Yoast SEO Premium we use the edit_form_advanced hook to output a hidden input field.
On a Gutenberg page, this hook is being triggered at initialization.

This caused us a very strange bug, where the metabox under the content has very unwanted behaviour.

This used to only be the case for IE, but since Gutenberg 4.1, this also affects Chrome (Firefox can handle it).

The problem is that the hidden field (which is output twice, for some odd reason), is being placed before the opening <html tag of the document.

As this behaviour is pretty obscure, I'm most certain that other plugins also use echo or print to the document in this hook, resulting in unwanted rending of the page.

As mentioned in #1352:

edit_form_advanced - Fires after 'normal' context meta boxes have been output for all post types other than 'page'.

To Reproduce

Add a hook implementation:

add_action( 'edit_form_advanced', function() {
  echo '<input type="hidden" name="some_name" value="something"/>';
}, 10, 1 );

This will create an invalid HTML document structure, which results in unreliable results in different browsers.

Expected behavior
I expect this hook to be called at the location where the content div has been rendered, not before any output is sent.

Screenshots
weird-behaviour

Desktop (please complete the following information):

  • OS: MacOS
  • Browser Chrome, IE
  • Version latest

Smartphone (please complete the following information):
Untested

Additional context
Gutenberg 4.1-RC2
@azaozz
Copy link
Contributor

azaozz commented Oct 23, 2018 •
edited

Uh, that seems a "classic" example of doing-it-wrong? :)
The filters on the TinyMCE init array are meant to filter that array, not to trigger output. For outputting extra HTML together the editor's HTML, most appropriate would be to use 'the_editor' filter which is meant for that. You can safely prepend or append any HTML there.

Edit: Ah, disregard the above. Not sure how I mixed edit_form_advanced filter with tiny_mce_before_init, seems I'm getting too tired :)

@swissspidy
Copy link
Member

edit_form_advanced is not a TinyMCE related filter though? 🤔

According to https://wpdirectory.net/search/01CTG7DNKS5FTNBZP6VY01EB6F many plugins print some custom HTML in edit_form_advanced, most notably https://wordpress.org/plugins/disable-comments/.

@designsimply designsimply added the Needs Technical Feedback Needs testing from a developer perspective. label Oct 23, 2018
@1000camels
Copy link

do_action( 'edit_form_advanced', $post ); is also in wp-admin/edit-form-advanced.php, where before it calls do_meta_boxes(null, 'normal', $post), and after it calls do_meta_boxes(null, 'advanced', $post), both of which output markup.
The other place that edit_form_advanced is called is in wp-includes/class-wp-embed.php, which also outputs markup.

So it sounds to me like gutenberg_collect_meta_box_data() should be calling some other action.

@moorscode
Copy link
Contributor Author

/cc @aduth is this on your radar?

@danielbachhuber danielbachhuber added Backwards Compatibility Issues or PRs that impact backwards compatability [Type] Bug An existing feature does not function as intended labels Oct 30, 2018
@danielbachhuber danielbachhuber added this to the WordPress 5.0 milestone Oct 30, 2018
@danielbachhuber
Copy link
Member

@moorscode Was this positively or negatively impacted by #10660?

@moorscode
Copy link
Contributor Author

@danielbachhuber negatively, it was not as broken before (there were strange visual glitches before).

@danielbachhuber
Copy link
Member

@moorscode Ok, thanks.

I think we need to keep #10660 in, because there were more significant issues caused by the prior implementation.

For this particular issue, one option to consider is:

ob_start();
do_action( 'edit_form_advanced', $post );
$_edit_form_advanced_output = ob_get_clean();

We'd then write $_edit_form_advanced_output to the page wherever appropriate.

@moorscode
Copy link
Contributor Author

Sounds like a good plan.

@danielbachhuber
Copy link
Member

Related https://core.trac.wordpress.org/ticket/45283

@mtias
Copy link
Member

mtias commented Nov 16, 2018

I'm closing this based on https://core.trac.wordpress.org/changeset/43882 . Please reopen if I'm wrongly assuming relationship.

@mtias mtias closed this as completed Nov 16, 2018
@garretthyder garretthyder mentioned this issue Nov 27, 2018
Closed
@garretthyder
Copy link
Contributor

garretthyder commented Nov 27, 2018 •
edited

Hi @mtias
I confirmed that the changeset you mentioned resolved this on 5.0 RC in the case of Disable Comments as it does something similar with injecting hidden inputs. I'm wondering if that change should be ported into the Gutenberg plugin as well for plugin-only users and users of 5.0 that have the Gutenberg plugin installed as those users will still experience this issue until they adopt WP5.0 and drop Gutenberg as a plugin.
Thank you

@garretthyder garretthyder mentioned this issue Dec 18, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability Needs Technical Feedback Needs testing from a developer perspective. [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
WordPress 5.0 RC
Development

No branches or pull requests
8 participants
@danielbachhuber @mtias @1000camels @azaozz @swissspidy @designsimply @moorscode @garretthyder