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.

Sign up for GitHub

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

Gutenberg: Add and use jetpack_register_block_type() wrapper #11310

Merged
merged 1 commit into from
Feb 12, 2019
Merged

Gutenberg: Add and use jetpack_register_block_type() wrapper #11310

merged 1 commit into from
Feb 12, 2019

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Feb 8, 2019 •
edited
Loading

This includes a function_exists() check to avoid usage of register_block_type in older WordPress versions that don't yet have it, leading to fatals (which my recent #11212 caused -- very sorry!)

Some convo (with @dereksmart and @jeherve) at p1549647686423800-slack-jetpack-gutenberg

Changes proposed in this Pull Request:

  • Add and use jetpack_register_block_type() wrapper instead of register_block_type()

Testing instructions:

  • Verify that blocks are still registered and available
  • On a test site running WP 4.9.9, verify that no fatals occur.
  • Grep for register_block_type to make sure we're not calling it directly anymore.

Follow-up

As a follow-up, we should modify tests/php/test_class.jetpack-gutenberg.php (and Jetpack_Gutenberg) so it won't skip tests on a WP installation without register_block_type to make sure that Jetpack_Gutenberg doesn't fatal.

Proposed changelog entry for your changes:

  • Add and use jetpack_register_block_type() wrapper instead of register_block_type()

@ockham ockham added [Status] In Progress [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Feb 8, 2019
@ockham ockham self-assigned this Feb 8, 2019
@ockham ockham requested a review from a team as a code owner February 8, 2019 18:33
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello ockham! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D24145-code before merging this PR. Thank you!

@jetpackbot
Copy link

jetpackbot commented Feb 8, 2019 •
edited
Loading

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: March 5, 2019.
Scheduled code freeze: February 26, 2019

Generated by 🚫 dangerJS against 86d3d34

@ockham ockham added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] In Progress labels Feb 8, 2019
enejb
enejb reviewed Feb 8, 2019
View reviewed changes
class.jetpack-gutenberg.php Outdated Show resolved Hide resolved
@jeherve jeherve added this to the 7.1 milestone Feb 8, 2019
@jeherve jeherve force-pushed the add/safe-register-block-type-function branch from 9509c9d to 3cf543a Compare February 8, 2019 20:58
@matticbot
Copy link
Contributor

ockham, Your synced wpcom patch D24145-code has been updated.

@jeherve jeherve changed the title Gutenberg: Add and use safe_register_block_type() wrapper Gutenberg: Add and use jetpack_safe_register_block_type() wrapper Feb 8, 2019
jeherve
jeherve previously approved these changes Feb 8, 2019
View reviewed changes
Copy link
Member

@jeherve jeherve 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 well for me. I only have one minor comment.

tests/php/sync/test_class.jetpack-sync-callables.php Outdated Show resolved Hide resolved
@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Feb 8, 2019
@ockham
Copy link
Contributor Author

ockham commented Feb 11, 2019

Thanks for adding the jetpack_ prefix, @jeherve -- makes sense to avoid collisions with other plugins that way.

I know I'm the one who suggested the safe_ infix, but I'm having second thoughts now - jetpack_safe_register_block_type is quite a mouthful 😅 Should we drop the safe_ after all? (jetpack_register_block_type won't collide with the other now-deprecated previous function which was fortunately called jetpack_register_block.)

@jeherve
Copy link
Member

jeherve commented Feb 11, 2019

Should we drop the safe_ after all?

Yeah, you're right. It seems safe without it. Let's drop it.

@matticbot
Copy link
Contributor

ockham, Your synced wpcom patch D24145-code has been updated.

@ockham ockham added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Feb 11, 2019
@ockham ockham changed the title Gutenberg: Add and use jetpack_safe_register_block_type() wrapper Gutenberg: Add and use jetpack_register_block_type() wrapper Feb 11, 2019
@ockham
Copy link
Contributor Author

ockham commented Feb 11, 2019

Changed the function name to jetpack_register_block_type and removed the now-obsolete function_exists() check from tests/php/sync/test_class.jetpack-sync-callables.php.

@ockham
Copy link
Contributor Author

ockham commented Feb 11, 2019

Putting on hold to get #11312 in first.

@ockham ockham added the DO NOT MERGE don't merge it! label Feb 11, 2019
@jeherve
Copy link
Member

jeherve commented Feb 11, 2019

This should now be good to rebase. 👍

@ockham ockham removed the DO NOT MERGE don't merge it! label Feb 11, 2019
@ockham ockham force-pushed the add/safe-register-block-type-function branch from a4e71b1 to 08e7bce Compare February 11, 2019 20:31
@ockham ockham force-pushed the add/safe-register-block-type-function branch from 08e7bce to 86d3d34 Compare February 11, 2019 20:37
@matticbot
Copy link
Contributor

ockham, Your synced wpcom patch D24145-code has been updated.

@ockham
Copy link
Contributor Author

ockham commented Feb 11, 2019

Rebased.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Feb 12, 2019
jeherve
jeherve approved these changes Feb 12, 2019
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

Looking good. Merge when ready.

@ockham ockham merged commit 4085d87 into master Feb 12, 2019
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Feb 12, 2019
@ockham ockham deleted the add/safe-register-block-type-function branch February 12, 2019 11:51
@jeherve jeherve mentioned this pull request Feb 12, 2019
Merged
@enejb enejb mentioned this pull request Feb 12, 2019
Merged
jeherve pushed a commit that referenced this pull request Feb 15, 2019
@enejb @jeherve
Gutenpack: Revert deprecating jetpack_register_block (#11328)
376b6d8 
We introduced `jetpack_register_block_type` in #11310. But since we already have a wrapper function that we also deprecated. We end up with 2 functions `jetpack_register_block_type` and `jetpack_register_block` that do the same thing. 
This PR removed the new wrapper function in favour of the old one and ads a check that we don't call jetpack_register_block_type without `jetpack/` which is what we expect. 

#### Changes proposed in this Pull Request:
* remove the newly added jetpack_register_block_type in favour of the old jetpack_register_block_type. 


#### Testing instructions:
* Do the tests still pass? 
* Do the blocks still load in the editor? 
* Do you notice any php errors/notices? 

#### Proposed changelog entry
I don't think this needs a changelog entry
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enejb enejb enejb left review comments

@jeherve jeherve jeherve approved these changes

Assignees

@ockham ockham

Labels
[Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files
Projects
None yet
Milestone
7.1
Development

Successfully merging this pull request may close these issues.
5 participants
@ockham @matticbot @jetpackbot @jeherve @enejb