Skip to content

Ensuring extensions/blocks do not inadvertently register twice#11618

Merged
kraftbj merged 6 commits intomasterfrom
fix/multiple-block-registration
Mar 25, 2019
Merged

Ensuring extensions/blocks do not inadvertently register twice#11618
kraftbj merged 6 commits intomasterfrom
fix/multiple-block-registration

Conversation

@gititon
Copy link
Copy Markdown
Contributor

@gititon gititon commented Mar 19, 2019

This PR ensures extensions/blocks cannot be registered more than once.

Fixes #11590
Fixes #11506

Changes proposed in this Pull Request:

Will not register an extension/block if it's already registered.

Testing instructions:

See #11590 (comment)
Also, an additional phpunit test checks this functionality

Proposed changelog entry for your changes:

No changelog entry needed

@gititon gititon added [Status] Needs Review This PR is ready for review. [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Mar 19, 2019
@gititon gititon self-assigned this Mar 19, 2019
@gititon gititon requested review from a team March 19, 2019 21:12
@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Mar 19, 2019

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: April 2, 2019.
Scheduled code freeze: March 26, 2019

Generated by 🚫 dangerJS against 22d5945

@jeherve jeherve added this to the 7.2 milestone Mar 19, 2019
@jeherve jeherve added Bug When a feature is broken and / or not performing as intended Simple Payments labels Mar 19, 2019
Copy link
Copy Markdown
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.

Awesome. This may also fix #11506.

I think you have a few changes in here that did not belong in this PR though. Could you take a look?

Thanks!

Comment thread sync/class.jetpack-sync-sender.php Outdated
Comment thread class.jetpack-gutenberg.php Outdated
Comment thread class.jetpack-gutenberg.php Outdated
@jeherve jeherve added [Status] In Progress and removed [Status] Needs Review This PR is ready for review. labels Mar 20, 2019
Comment thread sync/class.jetpack-sync-sender.php Outdated
lezama and others added 2 commits March 20, 2019 10:46
Co-Authored-By: gititon <gititon@users.noreply.github.com>
// Checking whether block is registered to ensure it isn't registered twice.
if ( Jetpack_Gutenberg::is_registered( $slug ) ) {
return false;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The thing is, this sort of check is kinda what register_block_type() does, and what results in the warning that we're trying to fix here (#11590) 😅 So in a way, we're just cutting in line before that check to silence that warning, when the underlying issue is that some consumer registers a block twice (which it shouldn't). So ideally, we'd find out what consumer that is and change it accordingly 😬

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ideally, we'd find out what consumer that is and change it accordingly

There is a use-case here for example:
#11506 (comment)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems like both are valid concerns but given the time constraints, this seems like a valid way to go for now?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@matticbot
Copy link
Copy Markdown
Contributor

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

@gititon gititon added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Mar 20, 2019
@gititon gititon requested a review from jeherve March 21, 2019 15:00
Copy link
Copy Markdown
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.

I'll let @ockham chime in with his opinion since he commented here:
https://github.com/Automattic/jetpack/pull/11618/files#r267412770

Until then, just a minor nitpicking / editor shouting.

Comment thread tests/php/bootstrap.php Outdated
@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Mar 21, 2019
Co-Authored-By: kraftbj <public@brandonkraft.com>
Copy link
Copy Markdown
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

I'll let @ockham chime in with his opinion since he commented here:
https://github.com/Automattic/jetpack/pull/11618/files#r267412770

I had another look at @simison's great analysis at #11590 and commented there: #11590 (comment)

tl;dr: Let's go with this approach for now 😄

@jeherve jeherve added [Feature] Related Posts [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Mar 22, 2019
@kraftbj kraftbj merged commit 6260213 into master Mar 25, 2019
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 25, 2019
kraftbj added a commit that referenced this pull request Mar 25, 2019
kraftbj added a commit that referenced this pull request Mar 27, 2019
* Initial Changelog for 7.2

* Testing list: add mention of IE11 testing

* Initial Changelog for 7.2

* Testing list: add mention of IE11 testing

* Add CL for #11224

* Add CL for #11426

* Add CL for #11442

* Add testing instructions for #11224

* Add CL for #11451

* Reclassify CL item

* Add testing instructions for #11451

* Add CL for #11486

* Add CL for #11418

* Add CL for #11524

* Add CL and testing instructions for #11449

* Add CL for #11460

* Add CL for #11520 and #11582

* Add CL for #11531

* Add CL #11644

* Add testing instructions for #11644

* Add testing instructions for #11644

* Add CL for #11618

* Uniform changelog lines

* CL #11679

* CL #11661

* CL #11654

* CL #11645

* CL #11643

* CL #11636

* CL #11635 and for other PHPCS commits

* CL #11627

* CL #11626

* CL #11598

* CL #11596

* Remove nested items for shortcopy. I don't believe the detailed list is helpful

* CL #11570

* CL #11569

* CL #11560

* CL #11558

* CL #11555

* CL #6704

* CL #11298

* CL #11324

* CL #11443

* CL #11484

* CL #11516

* CL #11529

* Expand Ads block enhancement CL item
@kraftbj kraftbj deleted the fix/multiple-block-registration branch May 24, 2019 17:22
@jeherve jeherve added the [Feature] Pay with PayPal aka Simple Payments label Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug When a feature is broken and / or not performing as intended [Feature] Pay with PayPal aka Simple Payments [Feature] Related Posts [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants