-
Notifications
You must be signed in to change notification settings - Fork 843
Forms: Remove classic Admin initialization and migrate functionality #46254
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
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
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 🤖 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 SummaryCoverage changed in 4 files.
Full summary · PHP report · JS report Coverage check overridden by
Coverage tests to be added later
|
| * @var string The nonce field name for GDrive export. | ||
| */ | ||
| private $export_nonce_field_gdrive = 'feedback_export_nonce_gdrive'; | ||
|
|
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.
I wonder if it makes any sense to move all the Google drive logic to the Google_Drive class that was created in August? That's in forms/src/services/.
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.
I guess it would? But I'm also on the side of sticking to what we have tested and worked well. We can always follow up or find optimizations, specially in terms of code readability and organization. I just wanted to stay as simple as possible, after all, this is janitorial and not meant to take so much of us.
That said, feel free to push changes and add tests or test instructions. Or create a PR branching out of this one if that helps as a "follow up reminder".
b02867b to
5e3b45a
Compare
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.
Pull request overview
This PR deprecates the classic Admin initialization in the Forms package by removing the Admin::init() call from Util::init() and migrating the Google Drive export functionality to the Contact_Form_Plugin class. The changes improve code organization by centralizing export functionality and fix a bug in Google Drive export where array indices weren't checked before access.
Key changes:
- Removed
Admin::init()call fromUtil::init()to stop initializing deprecated classic admin functionality - Migrated
export_to_gdrive()method fromAdminclass toContact_Form_Pluginwith improved array handling - Added comprehensive test coverage validating the deprecation and migration
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/packages/forms/src/contact-form/class-util.php | Removes the conditional call to Admin::init() to stop initializing classic admin functionality |
| projects/packages/forms/src/contact-form/class-admin.php | Marks init() and export_to_gdrive() as deprecated with delegation to new implementation; removes AJAX handler registrations for export functions |
| projects/packages/forms/src/contact-form/class-contact-form-plugin.php | Adds export_to_gdrive() method with improved array index checking and registers AJAX handler for Google Drive export |
| projects/packages/forms/src/contact-form/class-editor-view.php | Removes unnecessary removal of Admin media button action since Admin is no longer initialized |
| projects/packages/forms/tests/php/contact-form/Util_Test.php | Adds comprehensive tests for Admin deprecation, hook registration validation, export delegation, and security checks |
| projects/packages/forms/.phan/baseline.php | Updates Phan baseline to reflect deprecated function usage in tests and removed type mismatch suppression |
| projects/packages/forms/changelog/remove-forms-old-admin | Adds changelog entry for Forms package marking change as major/removed |
| projects/plugins/jetpack/changelog/remove-forms-old-admin | Adds changelog entry for Jetpack plugin marking change as major/bugfix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
5f9f218 to
88d3d32
Compare
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.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| add_action( 'admin_enqueue_scripts', array( $this, 'admin_enqueue_scripts' ) ); | ||
| add_action( 'admin_footer-edit.php', array( $this, 'print_export_modal' ) ); | ||
|
|
||
| add_action( 'wp_ajax_grunion_export_to_gdrive', array( $this, 'export_to_gdrive' ) ); | ||
| add_action( 'wp_ajax_grunion_gdrive_connection', array( $this, 'test_gdrive_connection' ) ); | ||
| } |
Copilot
AI
Dec 10, 2025
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.
The removal of the wp_ajax_grunion_gdrive_connection action registration breaks the Google Drive connection polling functionality. The JavaScript code in grunion-admin.js (line 219) still calls this AJAX action via startPollingConnection(), but the endpoint is no longer registered.
This action handler should be migrated to Contact_Form_Plugin class (similar to how export_to_gdrive was migrated), and the test_gdrive_connection method should either be migrated or deprecated with delegation.
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.
@CGastrell - I'm curious about this too. Do/will we still need the logic and hook for test_gdrive_connection?
a602e8e to
88d3d32
Compare
…p 7.2/3/4 seem to be failing at it
| ), | ||
| 200, | ||
| JSON_UNESCAPED_SLASHES | ||
| ); |
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.
Test results:
- Sanity check that menu items and dashboard work as expected
- Confirm CSV exports work
- Confirm Google Drive exports work
- Confirm that if I add code to call Admin::init() directly, I do get deprecation notices in debug.log
- Confirm tests pass
Notes:
- The PR description says "Fixes issue with CSV column array building to handle empty values properly by checking if array index exists before accessing", but I don't think the PR actually makes any changes for this?
- The testing instructions say go to Dashboard → Feedback → Select a form... but we don't load a Feedback menu item or page anymore. If I try to load that page directly, I'm just redirected to our new forms dashboard. Also, when on our forms dashboard, you can't select a form when on the Jetpack > Forms page. So I wasn't quite sure what this was asking?
This is a rebase of #44538 but decided to open a new PR so to not mess with the original changes.
Proposed changes:
Admin::init()method in the Forms package as part of migrating away from the classic admin interfaceAdmin::init()fromUtil::init()to stop initializing the deprecated classic admin functionalityAdminclass toContact_Form_Pluginclass where it belongsAdmin::init()is no longer called during initializationOther information:
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Test that Forms functionality still works:
Test that deprecated code triggers warnings (optional):
WP_DEBUGenabled, add this code to call the deprecated method:Test that classic admin is no longer initialized:
jetpack test projects/packages/forms -- --filter=Util_Test