-
Notifications
You must be signed in to change notification settings - Fork 801
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
Conversation
Caution: This PR has changes that must be merged to WordPress.com |
Thank you for the great PR description! When this PR is ready for review, please apply the Scheduled Jetpack release: July 2, 2019. |
@youknowriad @gziolo @aduth Is the original an acceptable usage, or do folks always need to wrap in a function? |
gwwar, Your synced wpcom patch D29394-code has been updated. |
gwwar, Your synced wpcom patch D29394-code has been updated. |
8b7fa41
to
51469f9
Compare
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 /> ); }, |
There was a problem hiding this comment.
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.
Verified the issue on my local Jetpack site ( However, I noticed more regressions possibly caused by the latest version of the Gutenberg plugin. It seems that all the Jetpack blocks using |
gwwar, Your synced wpcom patch D29394-code has been updated. |
Interesting. After changing some imports to pull out the dependencies from Maybe changes introduced in WordPress/gutenberg#15989 have caused the issue and we cannot longer import |
Since Jetpack supports current-1 WordPress version, we need to be careful on relying @jeherve how does the core version support for Jetpack v7.5 look like? Thanks for fixing it everyone! |
@simison You're correct. Jetpack supports WP current and -1.
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 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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Reopening since this is still broken with v5.9.2 |
fbde411
to
a475ada
Compare
gwwar, Your synced wpcom patch D29394-code has been updated. |
Did a quick audit, and it looks like this is the only block that doesn't pass an explicit save function. |
@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. |
There was a problem hiding this 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.
* Test if this fixes api save validation * Wrap InnerBlocks.Content in a function
Cherry-picked to |
Thanks @jeherve ! |
* 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
* 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
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:
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:
The "save" property must be a valid function
Proposed changelog entry for your changes: