Skip to content

Gutenberg Shim #745

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

Closed
wants to merge 21 commits into from
Closed

Gutenberg Shim #745

wants to merge 21 commits into from

Conversation

gfargo
Copy link

@gfargo gfargo commented Apr 29, 2019

  • Modifies fm_add_script JavaScript dependencies to include wp-edit-post when Gutenberg is active.
  • Includes new script to manually trigger FM fm_added_element event via wp.domReady

}

// Fallback if we don't have access to `current_screen`.
if ( ! $is_gutenberg_editor ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block will still run even when the screen is available if $current_screen->is_block_editor is false.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if $current_screen->is_block_editor is false, we'd hit the fallback which returns false also in my testing, which prevented anything from loading. 🤔 This seems like what it's supposed to do, no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I just mean that running the fallback seems redundant if WP_Screen already provided the answer.

Copy link
Contributor

@jomurgel jomurgel May 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right. I agree. Tbh I'm not certain why we have the fallback specifically since all posts should have access to it, but @gfargo might have more information for that.

The answer could be because of the note you left below:

Having said all that: I think fm_enqueue_scripts will also run on the frontend if any Fieldmanager assets are enqueued (via \Fieldmanager_Util_Assets::hook_enqueue()). If so, that could lead to cases where current_screen does not fire, but because the global post from get_the_ID() supports the block editor, wp-post-edit will be enqueued. That might be the intention here — maybe it's why get_the_ID() is used?

But I think in another note about how there is an is_admin() check-in \Fieldmanager_Util_Assets::add_script() to prevent loading on the front end may void that, and then the need for the fallback.

Testing locally, I don't actually find the need for the fallback, but not removing until there's a discussion about this from the author.


// Fallback if we don't have access to `current_screen`.
if ( ! $is_gutenberg_editor ) {
$post_id = false !== get_the_ID() ? get_the_ID() : $GLOBALS['post_ID'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The use of a template tag seems unexpected here. What is the desired value? The ID of the post being edited?
  • I think $GLOBALS['post_ID'] will exist only only in $_POST. For $_GET requests, the ID is in $_GET['post']. So, there is a possibility of undefined-index warnings, and I'm not sure whether the logic here captures all the needed cases.
  • Also, any value from a superglobal will need sanitization.
  • Having said all that: I think fm_enqueue_scripts will also run on the frontend if any Fieldmanager assets are enqueued (via \Fieldmanager_Util_Assets::hook_enqueue()). If so, that could lead to cases where current_screen does not fire, but because the global post from get_the_ID() supports the block editor, wp-post-edit will be enqueued. That might be the intention here — maybe it's why get_the_ID() is used?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but because the global post from get_the_ID() supports the block editor, wp-post-edit will be enqueued

Pardon me: Not enqueued, but added as a dependency. However, if a script dependency is not enqueued, is the dependency enqueued, or does the script fail to load?

@jomurgel
Copy link
Contributor

Made several updates here to tackle the following:

  • Add a fallback for an older version of WP to avoid errors trying to access functions/properties that don't exist.
  • Resolves PHPCS and PHPUnit errors.
  • Addressed several notes above.

@@ -0,0 +1,10 @@
( function( $ ) {
if (wp.domReady) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wp isn't always available when this fires.

@kevinfodness
Copy link
Member

Fixed via #757

@jomurgel jomurgel deleted the gutenberg-support branch November 2, 2021 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants