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

Gutenberg v5.9.x and Contact Form Block Support #12673

Merged
merged 2 commits into from
Jun 14, 2019
Merged

Conversation

gwwar
Copy link
Contributor

@gwwar gwwar commented Jun 12, 2019

Changes proposed in this Pull Request:

When using Gutenberg v5.9.0, the Jetpack contact form is not available. Existing blocks display the following in the editor:

Screen Shot 2019-06-12 at 3 11 48 PM

This PR wraps the save functions in a wrapper to satisfy this check: https://github.com/WordPress/gutenberg/pull/14529/files#diff-33e0ac9c44b16a617039a563d4c065f3R108

Testing instructions:

Screen Shot 2019-06-12 at 4 08 35 PM

  • Install Gutenberg v5.9.0 and activate
  • See if contact form is available in the inserter
  • No regressions in behavior
  • No error in console about The "save" property must be a valid function

Proposed changelog entry for your changes:

  • no changelog entry needed

@gwwar gwwar added [Feature] Contact Form [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Jun 12, 2019
@gwwar gwwar requested a review from a team as a code owner June 12, 2019 22:47
@gwwar gwwar self-assigned this Jun 12, 2019
@matticbot
Copy link
Contributor

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

@jetpackbot
Copy link

jetpackbot commented Jun 12, 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: July 2, 2019.
Scheduled code freeze: June 25, 2019

Generated by 🚫 dangerJS against a475ada

@gwwar
Copy link
Contributor Author

gwwar commented Jun 12, 2019

@youknowriad @gziolo @aduth Is the original an acceptable usage, or do folks always need to wrap in a function?

@gwwar gwwar requested review from a team June 12, 2019 23:19
@matticbot
Copy link
Contributor

gwwar, Your synced wpcom patch D29394-code has been updated.

@matticbot
Copy link
Contributor

gwwar, Your synced wpcom patch D29394-code has been updated.

@matticbot
Copy link
Contributor

gwwar, Your synced wpcom patch D29394-code has been updated.

@@ -69,7 +69,7 @@ export const settings = {
},

edit: JetpackContactForm,
save: InnerBlocks.Content,
save: function() { return ( <InnerBlocks.Content /> ); },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seeing same behavior here with function vs fat arrow, so this gets past the validation check, but there are regressions in behavior.

@mmtr
Copy link
Member

mmtr commented Jun 13, 2019

Verified the issue on my local Jetpack site (jetpack/contact-form cannot be inserted after installing Gutenberg v5.9.0) and tested that changes on this PR make the block insertable again.

However, I noticed more regressions possibly caused by the latest version of the Gutenberg plugin. It seems that all the Jetpack blocks using InnerBlocks.Content like jetpack/contact-form or jetpack/contact-info are currently broken, and none of the changes done to the blocks in the editor are saved to the post content.

@matticbot
Copy link
Contributor

gwwar, Your synced wpcom patch D29394-code has been updated.

@mmtr
Copy link
Member

mmtr commented Jun 13, 2019

Interesting. After changing some imports to pull out the dependencies from @wordpress/block-editor instead of from the deprecated @wordpress/editor, the regression I was experiencing has been fixed.

Maybe changes introduced in WordPress/gutenberg#15989 have caused the issue and we cannot longer import InnerBlocks from @wordpress/editor?

@simison
Copy link
Member

simison commented Jun 13, 2019

Since Jetpack supports current-1 WordPress version, we need to be careful on relying @wordpress/block-editor as it might not be available.

@jeherve how does the core version support for Jetpack v7.5 look like?

Thanks for fixing it everyone!

@jeherve jeherve added [Type] Bug When a feature is broken and / or not performing as intended [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack and removed [Status] In Progress labels Jun 13, 2019
@jeherve jeherve added this to the 7.5 milestone Jun 13, 2019
@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 Jun 13, 2019
@jeherve
Copy link
Member

jeherve commented Jun 13, 2019

@simison You're correct. Jetpack supports WP current and -1.

how does the core version support for the v7.5 look like?

We'll need to keep supporting WP 5.1 in Jetpack until WP 5.3 is released, so Jetpack 7.5 will need to support WP 5.1.

Since @wordpress/block-editor was added in WP 5.2 (ref), we can't rely on it just yet.

If this bug impacts all blocks relying on Inner Blocks, would it be an option to fix the issue in Gutenberg instead, so all plugins currently using Inner Blocks would be fixed at once?

@@ -69,7 +69,9 @@ export const settings = {
},

edit: JetpackContactForm,
save: InnerBlocks.Content,

Choose a reason for hiding this comment

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

It does seem to me that this is a valid usage as we always considered save as a "component". Should we update the check function @gziolo?

Copy link
Member

Choose a reason for hiding this comment

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

In getSaveElement we have https://github.com/WordPress/gutenberg/blob/master/packages/blocks/src/api/serializer.js#L68-L70:

	// Component classes are unsupported for save since serialization must
	// occur synchronously. For improved interoperability with higher-order
	// components which often return component class, emulate basic support.

We provide a workaround, so we should relax this check. I will open PR soon.

Copy link
Member

@gziolo gziolo Jun 13, 2019

Choose a reason for hiding this comment

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

It seems like this can't be the issue as we have the same check for save method almost forever:
https://github.com/WordPress/gutenberg/blob/da8184a055d8b2d40ba0306faf7103c0a961726a/blocks/api/registration.js#L96-L101

	if ( ! settings || ! isFunction( settings.save ) ) {
		console.error(
			'The "save" property must be specified and must be a valid function.'
		);
		return;
	}

My bet that is that the way deprecation works for InnerBlocks in @wordpress/editor, it doesn't propagate Content further, we need to fix it. It's clearly undefined when loaded in Jetpack. I'm sure that it will break other plugins as well.

Copy link

@youknowriad youknowriad Jun 13, 2019

Choose a reason for hiding this comment

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

@gziolo Oh, that's a bigger issue that we may even consider for 5.2.x.

Update: I guess the deprecation were only introduced in 5.9 which means we're good sorry for the alarm :)

Copy link
Member

Choose a reason for hiding this comment

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

Opened WordPress/gutenberg#16152 to address the issue where statics were not hoisted for deprecated components. This should fix the issue which happens here.

Copy link
Member

Choose a reason for hiding this comment

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

In my testing, you will have to use function expression for this block for safety. It's fragile to assume that InnerBlocks.Content will always be a function. The same applies to all save implementations. Even if we allow objects, it still might work differently than intended.

Related Slack discussion on WordPress.org: https://wordpress.slack.com/archives/C02QB2JS7/p1560500199068200

Copy link

@youknowriad youknowriad Jun 14, 2019

Choose a reason for hiding this comment

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

@gwwar I think we might still need this change in Jetpack because the component is not a function anymore.

@gwwar gwwar closed this Jun 14, 2019
@gwwar gwwar deleted the fix/contact-form-g-5-9 branch June 14, 2019 16:09
@gwwar gwwar restored the fix/contact-form-g-5-9 branch June 14, 2019 16:26
@gwwar
Copy link
Contributor Author

gwwar commented Jun 14, 2019

Reopening since this is still broken with v5.9.2

@gwwar gwwar reopened this Jun 14, 2019
@gwwar gwwar changed the title Gutenberg v5.9 and Contact Form Block Support Gutenberg v5.9.x and Contact Form Block Support Jun 14, 2019
@matticbot
Copy link
Contributor

gwwar, Your synced wpcom patch D29394-code has been updated.

@gwwar
Copy link
Contributor Author

gwwar commented Jun 14, 2019

Did a quick audit, and it looks like this is the only block that doesn't pass an explicit save function.

@gwwar gwwar removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Jun 14, 2019
@gwwar
Copy link
Contributor Author

gwwar commented Jun 14, 2019

@Automattic/jetpack-crew This is ready for review. I think we need a point release to fix this for folks using the Gutenberg plugin. I'll be proposing process updates for next time to avoid this in the future.

@jeherve jeherve added the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Jun 14, 2019
@jeherve jeherve modified the milestones: 7.5, 7.4.1 Jun 14, 2019
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, and there are no other blocks impacted by this. Merging.

@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 Jun 14, 2019
@jeherve jeherve merged commit e2bd57f into master Jun 14, 2019
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jun 14, 2019
@jeherve jeherve deleted the fix/contact-form-g-5-9 branch June 14, 2019 18:45
jeherve pushed a commit that referenced this pull request Jun 14, 2019
* Test if this fixes api save validation

* Wrap InnerBlocks.Content in a function
@jeherve
Copy link
Member

jeherve commented Jun 14, 2019

Cherry-picked to branch-7.4 in a22fe7f

@gwwar
Copy link
Contributor Author

gwwar commented Jun 14, 2019

Thanks @jeherve !

jeherve added a commit that referenced this pull request Jun 14, 2019
jeherve added a commit that referenced this pull request Jun 14, 2019
* Initial 7.5 Changelog entry.

- Clear Testing list.
- Update stable tag in readme.
- Move changelog to changelog.txt
- Clear readme.

* Changelog: add base for 7.4.1, and #12673
jeherve added a commit that referenced this pull request Jun 14, 2019
* Initial 7.5 Changelog entry.

- Clear Testing list.
- Update stable tag in readme.
- Move changelog to changelog.txt
- Clear readme.

* Changelog: add base for 7.4.1, and #12673
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Contact Form [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants