-
Notifications
You must be signed in to change notification settings - Fork 814
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
Update Text Field block so that it doesn't render a shortcode #42135
base: trunk
Are you sure you want to change the base?
Conversation
…nder using shortcodes Use the field transforms util in the child-blocks file too Add field text block v1 changelog
Are you an Automattician? The PR will need to be tested on WordPress.com. This comment will be updated with testing instructions as soon the build is complete. |
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCannot generate coverage summary while tests are failing. 🤐 Please fix the tests, or re-run the Code coverage job if it was something being flaky. |
This PR isn't intended to be merged. This is the first attempt at a proof of concept for #41840. The code is a little messy in places, and it ideally needs some tests! If we want to proceed with this approach, I think it'd be best to break this up into 2-3 smaller PRs where it's possible to focus more on code quality, testing and thorough review.
This demonstrates the possibility for a field block to no longer depend on shortcode rendering.
The good news:
Migrating individual blocks like this should be fairly straightforward once a foundation is put in place.
The bad news:
The form submission and verification code was fairly deeply coupled to the idea of all form fields being shortcodes, I had to refactor a significant part of it to remove its dependency on
Contact_Form
andContact_Form_Field
shortcode classes. The reasoning is explained some more in this comment - #41840 (comment).Things that haven't been addressed in this PR:
id
system for forms. I expect this won't be an issue to implement, it's just a fair amount of boilerplate, and it takes some time to test it all.phpcs:disable WordPress.Security.NonceVerification.Missing
without checking. Just like the code in trunk.Proposed changes:
projects/packages/forms/src/blocks/field-text/class-field-text-block.php
Contact_Form::parse_contact_field
. I've copied mostly what the shortcode does, but the code itself is a little different.Contact_Form
class handling form submission, this is now handled by a newContact_Form_Submission
class.Contact_Form
class handles submissions, and it gathers submission values and errors from every instance of aContact_Form_Field
.Contact_Form_Submission
instance is created, and all fields add their values and errors to this submission instance.process_submission
method is moved fromContact_Form
toContact_Form_Submission
, and it now interacts with these stored values/errors, instead of trying to retrieve them from theContact_Form
andContact_Form_Field
classes. A lot of this is copy/paste. Several utility functions are also moved over.Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions: