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

Allow pulled posts to use block editor #581

Merged

Conversation

davidmpurdy
Copy link
Contributor

@davidmpurdy davidmpurdy commented May 5, 2020

Please forgive me if any of the following isn't filled in well enough - this is my first "live" pull request and I'm figuring it out as I go (and any suggestions for getting better at this are welcome of course!).

Description of the Change

A call to get_post($post) returns a bool(false) if the $post doesn't have an ID on the site yet.

It looks to me like is_using_gutenberg is called on new posts before they have an ID on the local site in WordPressExternalConnection->to_wp_post (I think the post object being passed still has the post ID from the source site). Since new posts (without a local ID yet) will always fail the is_using_gutenberg test, new posts pulled in never use the block editor.

Alternate Designs

I don't have the time or the familiarity with the code base (although I'm working on getting there) to feel comfortable proposing another design. Part of the reason I suggest this one is that it looks like the call to get_post($post) was added relatively recently, which makes me think that since it worked without it previously it might work well without that specific call again.

Benefits

Pulled posts can use the block editor.

Possible Drawbacks

I don't see any, but I'm not familiar with all the reasons the call to get_post($post) was added (the note on the commit talks about copying it in from the core use_block_editor_for_post function, which makes me think it might not be necessary here, where the post won't always exist yet).

Verification Process

I tested it on a couple of local sites that I have set up and have been using to test pulling in remote content.

Edit: it looks like it failed some of the tests, but I'm afraid I don't fully understand the reasons. (Travis CI shows what appears to be an error saying "Could not setup WP Snapshots repository" and I'm afraid I don't understand the testing system well enough to troubleshoot it. If anyone can point me in the right direction, I'd be happy to try and figure out why this pull request isn't passing the tests.)

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Applicable Issues

Changelog Entry

…if the post didn't have an ID yet, causing Gutenberg to never be enabled for new posts
@davidmpurdy davidmpurdy changed the title Removed call to get_post in Gutenberg check that was returning false … Allow pulled posts to use block editor May 5, 2020
@jeffpaul jeffpaul added the type:bug Something isn't working. label May 7, 2020
@jeffpaul jeffpaul added this to the 2.0.0 milestone May 7, 2020
@jeffpaul jeffpaul requested a review from dinhtungdu May 7, 2020 02:49
@jeffpaul
Copy link
Member

jeffpaul commented May 7, 2020

@davidmpurdy welcome to Distributor and thanks for the PR, its greatly appreciated! For future reference, it's ideal to open an issue first to discuss the concern at hand and ideally agree upon an approach before diving into a resolution in a PR. Back to this issue/PR, I'm milestoning this for our 2.0.0 release as we're in the final stages of getting 1.6.0 released and am working to clear that milestone to prepare for release. We'll get back to you after this PR gets through review and let you know if we have any questions or concerns on your approach.

As a sidebar, I'm a big fan of public media and hope that all is well up in Alaska and with KTOO and thanks again for the PR... cheers!

@davidmpurdy
Copy link
Contributor Author

Thank you so much for the tip! I was trying to be helpful by bringing a solution instead of a problem - I will make sure to open an issue beforehand next time. Would it be helpful to open an issue now after the fact? (Is this a common best practice on GitHub or does it vary by project?)

Thank you for milestoning this PR and for the wonderful plugin!

@jeffpaul
Copy link
Member

@davidmpurdy no worries on an issue, the PR here is very well detailed though its appreciated in the future to have an issue open for discussion/agreement on approach.

@jeffpaul jeffpaul modified the milestones: 2.0.0, 1.7.0 Jan 15, 2021
@dkotter
Copy link
Collaborator

dkotter commented Jan 22, 2021

@davidmpurdy Thanks for this PR (and sorry for the very late reply here). I ran into this same issue myself and while thinking through how to solve this, I decided to see if there was an existing issue or PR already opened and found this one.

You are correct that using is_using_gutenberg here will always fail because the post has not been created yet. We could remove that get_post check but I think there's a better approach.

If an external item supports Gutenberg, it will contain the flag is_using_gutenberg. We can use that to determine if the item supports Gutenberg. In this case, checking that and then also using is_using_gutenberg just tells us the same thing, so it's kind of pointless to run that function.

What this code is trying to achieve is checking if both sites support Gutenberg but right now we are (unintentionally) just checking the external item. That said, we do have a utility function that checks if a post type supports Gutenberg which I think will be the right approach to use here.

Within that to_wp_post method, we should switch from using is_using_gutenberg to dt_use_block_editor_for_post_type and make sure we pass in the post type of that external item. So something like going from this:

Utils\is_using_gutenberg( new \WP_Post( $obj ) ) && isset( $post['is_using_gutenberg'] )

to this:

Utils\dt_use_block_editor_for_post_type( $obj->post_type ) && isset( $post['is_using_gutenberg'] )

Again, apologies for the late reply here so let me know if you'd still like to fix this or if you'd like me to make those changes. Thanks!

@dkotter dkotter self-requested a review January 22, 2021 23:06
…origin/allow-blocks-with-classic-editor-plugin
…ad of trying to check external posts for is_using_gutenberg (which will always return false for external posts since they don't have a local ID yet)
…g false if the post didn't have an ID yet, causing Gutenberg to never be enabled for new posts"

This reverts commit f99f514.
@davidmpurdy
Copy link
Contributor Author

@dkotter Thank you for your note and giving me the opportunity to update the PR.

I have update it as you described and tested it locally to make sure it works when pulling a Gutenberg post from another site.

Copy link
Contributor

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

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

This works great for me! Thank you, David!

@jeffpaul jeffpaul modified the milestones: 1.7.0, 1.6.3 Mar 3, 2021
@jeffpaul jeffpaul merged commit 36fc6e4 into 10up:develop Mar 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Something isn't working.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants