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.

Sign up for GitHub

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

Jump to bottom

Search: Add a hook on returning filters. #10706

Merged
merged 1 commit into from
Dec 19, 2018
Merged

Search: Add a hook on returning filters. #10706

merged 1 commit into from
Dec 19, 2018

Conversation

jenkoian
Copy link
Contributor

Fixes N/A(?)

Changes proposed in this Pull Request:

I recently had a use case where I wanted to add custom filters (i.e. not
a taxonomy, post type or date histogram) and found I could set the
filter no problem but get_filters() would ignore it as it wasn't
supported.

I'd like to be able to easily support custom filters through a hook into
get_filters().

Testing instructions:

  • Add custom filters using set_filters() e.g.
Jetpack_Search::instance()->set_filters(
	[
		'Price Range' => [
			'type' => 'range',
			'label' => 'price_range',
			'field' => 'price',
			'ranges' => $ranges,
			'count' => count( $ranges ),
		],
	]
);

Call get_filters() and observe that the added (unsupported) filter is not available.

Proposed changelog entry for your changes:

  • Added hook to get_filters() to allow the use of custom filters.

@jenkoian jenkoian requested a review from a team November 22, 2018 11:28
@jetpackbot
Copy link

jetpackbot commented Nov 22, 2018 •
edited
Loading

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: November 26, 2018.
Scheduled code freeze: November 19, 2018

Generated by 🚫 dangerJS

@jeherve jeherve added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Feature] Search For all things related to Search [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 fellow Jetpack developers. Label will be renamed soon. labels Nov 22, 2018
jeherve
jeherve requested changes Nov 22, 2018
View reviewed changes
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.

A few minor changes in the docblock.

modules/search/class.jetpack-search.php
/**
* Modify the filters returned by get_filters()
*
* @module search
Copy link
Member

Choose a reason for hiding this comment

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

We're passed the code freeze for 6.8.0, so you'll need to update this to 6.9.0 I'm afraid.

modules/search/class.jetpack-search.php Outdated
@@ -1610,7 +1610,17 @@ public function get_filters( WP_Query $query = null ) {
} // End foreach().
} // End foreach().

return $aggregation_data;
/**
* Modify the filters returned by get_filters()
Copy link
Member

Choose a reason for hiding this comment

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

Could you expand on that title a bit so we know what filters this is referring to? That information will end up being displayed in our codex, so it's important to be as detailed as possible for anyone looking for that filter in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some more info, let me know if that's ok. 👍

Search: Add a hook on returning filters.
03d8502 
I recently had a use case where I wanted to add custom filters (i.e. not
a taxonomy, post type or date histogram) and found I could set the
filter no problem but get_filters would ignore it as it wasn't
supported.

I'd like to be able to easily support custom filters through a hook into
get_filters().
@jeherve jeherve added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Nov 22, 2018
@jeherve jeherve requested review from xyu and bperson November 22, 2018 11:45
@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 fellow Jetpack developers. Label will be renamed soon. labels Nov 22, 2018
jeherve
jeherve approved these changes Nov 22, 2018
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks good to me. @xyu Could you take a look as well?

Thanks!

@bperson
Copy link

bperson commented Nov 22, 2018

This looks reasonable to me, if this is not urgent, we might want to wait for @gibrown to be back next week?

PS: @xyu is in sabbatical right now.

@bperson bperson requested a review from gibrown November 22, 2018 13:49
@jeherve
Copy link
Member

jeherve commented Nov 22, 2018

@bperson Thanks for jumping in! My bad! It can definitely wait, since we are in code freeze we are not going to be able to include this in the next release anyway.

@jenkoian
Copy link
Contributor Author

Any update?

@jeherve
Copy link
Member

jeherve commented Dec 17, 2018

@jenkoian The PR is marked as Ready to Merge, so it will be included in the next Jetpack release, scheduled for the beginning of January. We'll merge the PR before the code freeze and it will be included in the Beta that will be available a week before the stable release.

@jenkoian
Copy link
Contributor Author

@jeherve thanks for the info :)

@matticbot
Copy link
Contributor

D22545-code. (newly created revision)

@jeherve jeherve merged commit cbd728c into Automattic:master Dec 19, 2018
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Dec 19, 2018
@jeherve jeherve added this to the 6.9 milestone Dec 19, 2018
jeherve added a commit that referenced this pull request Dec 21, 2018
@jeherve
Changelog: add #10706
6b34f0e
jeherve added a commit that referenced this pull request Jan 3, 2019
@jeherve
Changelog: add #10706
cddc6e2
jeherve added a commit that referenced this pull request Jan 3, 2019
@jeherve
Jetpack 6.9: Changelog and testing list (#10722)
8d91603 
* Add first version of the Changelog and testing list for 6.9

* Changelog: add #10710

* changelog: add #10538

* changelog: add #10741

* changelog: add #10749

* changelog: add #10664

* changelog: add #10224

* changelog: add #10788

* Changelog: add #10560

* Chanegelog: add #10812

* changelog: add #10556

* Changelog: add #10668

* Changelog: add #10846

* Changelog: add #10947

* Changelog: add #10962

* Changelog: add #10956

* Changelog: add #10940

* Changelog: add #10934

* Changelog: add #10912

* changelog: add #10866

* changelog: add #10924

* Changelog: add #10936

* Changelog: add #10833

* changelog: add #10867

* Changelog: add #10960

* Changelog: add #10888

* changelog: add #10840

* changelog: add #10972

* Changelog: add #10979

* changelog: add #10909

* Changelog: add #10958

* Changelog: add #10981

* Changelog: add #10564

* Changelog: add #10809

* Changelog: add #10982

* Changelog: add #10706

* Changelog: add #10978

* Changelog: add #10132

* Changelog: add #11022

* Changelog: add #11024

* Changelog: add #10875

* Changelog: add #11030

* Changelog: add #11053

* Changelog: add #10880

* Changelog: add #9359

* Changelog: add #11037

* Update block list

* Changelog: add #11060

* Changelog: add #10755

* changelog: add #11000

* Changelog: add #10786

* Changelog: add #10945

* Changelog: add #10597
gibrown pushed a commit that referenced this pull request Nov 8, 2019
@jeherve @gibrown
Search: Add a hook on returning filters. (jetpack:10706)
4489043 
Summary:
 <!--- Provide a general summary of your changes in the Title above -->

 Fixes N/A(?)

 #### Changes proposed in this Pull Request:
 <!--- Explain what functional changes your PR includes -->

 I recently had a use case where I wanted to add custom filters (i.e. not
 a taxonomy, post type or date histogram) and found I could set the
 filter no problem but `get_filters()` would ignore it as it wasn't
 supported.

 I'd like to be able to easily support custom filters through a hook into
 `get_filters()`.

 #### Testing instructions:
 <!-- Please include detailed testing steps, explaining how to test your change. -->
 <!-- Bear in mind that context you working on is not obvious for everyone.  -->
 <!-- Adding "simple" configuration steps will help reviewers to get to your PR as quickly as possible. -->
 <!-- "Before / After" screenshots can also be very helpful when the change is visual. -->

 * Add custom filters using `set_filters()` e.g.

 ```php
 Jetpack_Search::instance()->set_filters(
 	[
 		'Price Range' => [
 			'type' => 'range',
 			'label' => 'price_range',
 			'field' => 'price',
 			'ranges' => $ranges,
 			'count' => count( $ranges ),
 		],
 	]
 );
 ```

 Call `get_filters()` and observe that the added (unsupported) filter is not available.

 #### Proposed changelog entry for your changes:
 <!-- Please do not leave this empty. If no changelog entry needed, state as such. -->

 * Added hook to `get_filters()` to allow the use of custom filters.

--

Automatically created by Jetpack Fusion from a Pull Request:
#10706

Test Plan: - [ ] Dummy test plan.

Reviewers: benoitperson, hypertextranch, gibrown, github-phab-bot

Tags: #touches_jetpack_files

Differential Revision: https://[private link]

Merges r185368-wpcom.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeherve jeherve jeherve approved these changes

@xyu xyu Awaiting requested review from xyu

@bperson bperson Awaiting requested review from bperson

@gibrown gibrown Awaiting requested review from gibrown

Assignees
No one assigned
Labels
[Feature] Search For all things related to Search Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
6.9
Development

Successfully merging this pull request may close these issues.
5 participants
@jenkoian @jetpackbot @bperson @jeherve @matticbot