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

Search: avoid registering the widget when the module is not active #21588

Merged
merged 2 commits into from
Nov 24, 2021

Conversation

jeherve
Copy link
Member

@jeherve jeherve commented Oct 29, 2021

Fixes #21587

Changes proposed in this Pull Request:

The Jetpack_Search class will not be loaded if the Search module is not active. So let's not register the widget when the feature is not available.

Jetpack product discussion

  • N/A

Does this pull request change what data or activity we track or use?

  • No

Testing instructions:

  • On a site connected to WordPress.com, where you have a plan but where the Search module is inactive.
  • Go to Jetpack > Settings and enable Extra Sidebar Widgets
  • Go to Appearance > Widgets
  • You should not see any errors.
  • Activate the Search feature.
  • You should be able to use the Jetpack Search Widget.

Fixes #21587

The `Jetpack_Search` class will not be loaded if the Search module is not active. So let's not register the widget when the feature is not available.
@jeherve jeherve added this to the jetpack/10.4 milestone Oct 29, 2021
@jeherve jeherve added [Feature] Extra Sidebar Widgets Instant Search [Feature] Search For all things related to Search [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Pri] Normal [Type] Bug When a feature is broken and / or not performing as intended labels Oct 29, 2021
@jeherve jeherve self-assigned this Oct 29, 2021
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello jeherve! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D69277-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Oct 29, 2021

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ All commits were linted before commit.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

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.


Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.


Jetpack plugin:

  • Next scheduled release: December 7, 2021.
  • Scheduled code freeze: November 30, 2021.

@gibrown
Copy link
Member

gibrown commented Oct 29, 2021

FYI, this was intentional for classic search so that the user could activate search by just adding the widget. But I think with instant search it is no longer needed.

@kangzj
Copy link
Contributor

kangzj commented Nov 1, 2021

Hi @jeherve Thanks for the PR. The logic look fine - but when I tested on this branch and master, both worked. That is to say, when I disabled search module, on both branches the Search (Jetpack) widget became unavailable. When I looked further, it seemed the file widgets/search.php is not loaded when search module is disabled. But I haven't looked the mechanism yet. Any idea? Thanks.

@jeherve
Copy link
Member Author

jeherve commented Nov 2, 2021

when I disabled search module, on both branches the Search (Jetpack) widget became unavailable.

Hm, that is an odd one. I've only tested this using the test site in p8oabR-K9-p2#comment-5572 so I do not really have more details I'm afraid.

@samiff samiff self-requested a review November 24, 2021 18:15
@samiff
Copy link
Contributor

samiff commented Nov 24, 2021

I was able to see the fatal Jeremy had mentioned with the search module deactivated:

[24-Nov-2021 18:37:59 UTC] PHP Fatal error:  Uncaught Error: Class 'Jetpack_Search' not found in /usr/local/src/jetpack-monorepo/projects/plugins/jetpack/modules/widgets/search.php:428
Stack trace:
#0 /usr/local/src/jetpack-monorepo/projects/plugins/jetpack/modules/widgets/search.php(301): Jetpack_Search_Widget->widget_instant()
#1 /var/www/html/wp-includes/widgets.php(1224): Jetpack_Search_Widget->widget()
#2 /var/www/html/wp-includes/rest-api/endpoints/class-wp-rest-widget-types-controller.php(520): the_widget()
#3 /var/www/html/wp-includes/rest-api/endpoints/class-wp-rest-widget-types-controller.php(490): WP_REST_Widget_Types_Controller->get_widget_preview()
#4 /var/www/html/wp-includes/rest-api/class-wp-rest-server.php(1140): WP_REST_Widget_Types_Controller->encode_form_data()
#5 /var/www/html/wp-includes/rest-api/class-wp-rest-server.php(987): WP_REST_Server->respond_to_request()
#6 /var/www/html/wp-includes/rest-api/class-wp-rest-server.php(414): WP_REST_Server->dispatch()
#7 /var/www/html/wp-includes/rest-api.php(370): WP_REST_Server->serve_request()
#8 /var/www/html/wp-includes/class-wp-hook.php(303): rest_api_loaded()
#9 /var/www/html/wp-includes/class-wp-hook.php(327): WP_Hook->apply_filters()
#10 /var/www/html/wp-includes/plugin.php(518): WP_Hook->do_action()
#11 /var/www/html/wp-includes/class-wp.php(388): do_action_ref_array()
#12 /var/www/html/wp-includes/class-wp.php(750): WP->parse_request()
#13 /var/www/html/wp-includes/functions.php(1291): WP->main()
#14 /var/www/html/wp-blog-header.php(16): wp()
#15 /var/www/html/index.php(17): require('/var/www/html/w...')
#16 {main}
  thrown in /usr/local/src/jetpack-monorepo/projects/plugins/jetpack/modules/widgets/search.php on line 428

With this PR patched, the fatal is fixed.

When I looked further, it seemed the file widgets/search.php is not loaded when search module is disabled.

If the search module is deactivated, then jetpack/modules/search.php (the module index file) won't load which is to be expected. However, with Extra Sidebar Widgets active, then the jetpack/modules/widgets/search.php file is being included, which has that jetpack_search_widget_init() call outside its main class.

function jetpack_load_widgets() {
$widgets_include = array();
foreach ( Jetpack::glob_php( dirname( __FILE__ ) . '/widgets' ) as $file ) {
$widgets_include[] = $file;
}
/**
* Modify which Jetpack Widgets to register.
*
* @module widgets
*
* @since 2.2.1
*
* @param array $widgets_include An array of widgets to be registered.
*/
$widgets_include = apply_filters( 'jetpack_widgets_to_include', $widgets_include );
foreach( $widgets_include as $include ) {
include_once $include;
}

I also did a check with all of the modules deactivated, but didn't see any other PHP errors when editing widgets, just some JS notices of block registration failures do to missing modules.

@samiff samiff merged commit 8636bc3 into master Nov 24, 2021
@samiff samiff deleted the fix/search-widget-fatal-module branch November 24, 2021 20:05
@github-actions
Copy link
Contributor

Great news! One last step: head over to your WordPress.com diff, D69277-code, and commit it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

@samiff
Copy link
Contributor

samiff commented Nov 24, 2021

r235602-wpcom

@kangzj
Copy link
Contributor

kangzj commented Nov 24, 2021

@samiff Thanks for looking into this, really appreciate that 👍

davidlonjon added a commit that referenced this pull request Nov 25, 2021
# By Jeremy Herve (5) and others
# Via GitHub
* master: (26 commits)
  Colors: update Primary green reference to match latest brand book (#21741)
  JS Connection: Moves registerSite logic to the store (#21732)
  Search: Add essential scaffolding for package (#21814)
  Search: avoid registering the widget when the module is not active (#21588)
  Add Video Tracks Support to VideoPress Block (#21578)
  Add deprecated to VideoPress block (#21802)
  Admin Menu: Make API tests compatible with WPCOM (#21850)
  External Media: Short-circuit requests earlier in the stack (#21854)
  Add Busy State to License Activation Flow Button (#21861)
  Fixed an issue with screen resolutions of under 783px that caused the content to not be properly displayed when the nav-unification is expanded on wp-admin. (#21869)
  E2E tests: migrate from Jest to Playwright runner (#21848)
  Update reCAPTCHA constants to match Google's Verbage (#12289)
  JITM: Sideload Jetpack Boost functionality  (#21860)
  Connection: properly add GET-parameters for the `fetchAuthorizationUrl` API call (#21750)
  License Flow: Assorted Styling Improvements (#21859)
  JITM: Sideload Jetpack Backup (#21719)
  Widget Visibility: Apply widget filtering to customizer preview (#21505)
  jetpack: Avoid generating unused JS for static-site-generator assets (#21789)
  Nav Unification: Support absolute URLs in upsell nudges (#21821)
  RePublicize: Enable the block editor UI by default (#21855)
  ...

# Conflicts:
#	projects/plugins/boost/tests/e2e/lib/env/prerequisites.js
#	projects/plugins/boost/tests/e2e/lib/setupTests.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Extra Sidebar Widgets [Feature] Search For all things related to Search [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Pri] Normal 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.

Search: Fatal Error when the Search module is inactive
5 participants