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

Updated the default search form to allow custom sorting and filtering. #1363

Closed
wants to merge 1 commit into from

Conversation

nglasl
Copy link

@nglasl nglasl commented Jan 12, 2016

This allows the ability to customise how your full-text search is both sorted and filtered.

@nglasl
Copy link
Author

nglasl commented Jan 12, 2016

I should probably also mention that this is required for both 3.1 and 3.2. Should I open these as separate pull requests? The main difference being DB::get_conn and DB::getConn.

@@ -112,7 +112,7 @@ public function getClassesToSearch() {
* @param array $data Request data as an associative array. Should contain at least a key 'Search' with all searched keywords.
* @return SS_List
*/
public function getResults($pageLength = null, $data = null){
Copy link
Contributor

Choose a reason for hiding this comment

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

A problem with this is that it's not going to be backwards compatible; If a user has subclassed SearchForm and overridden this method, upgrading to 3.4 would result in a strict method signature mismatch.

A hack you can use to add new properties is to leave the method signature, and use func_get_args() to get new parameters, but still add them to the PHPDoc. You will need to make sure you cross-PR this change to the master branch without the hack in place, though, and make sure any PR you make to the 3 branch comments the nature of this feature.

We can do breaking changes such as this in master, just not in 3.x, because of semver. :)

@tractorcow
Copy link
Contributor

I should probably also mention that this is required for both 3.1 and 3.2. Should I open these as separate pull requests? The main difference being DB::get_conn and DB::getConn.

We can't add any new features to 3.1 or 3.2 since these branches accept bugfixes only, sorry.

@nglasl
Copy link
Author

nglasl commented Jan 12, 2016

I did consider backwards compatibility with this, however I didn't quite analyse the case where someone might extend the search form, so I completely understand where you're coming from. Would it also be possible to leave it as is, however add a function with the old signature that passes to this?

In terms of the hack that you suggested (or the above), you don't think it's possible to get something like that back into 3.1 and 3.2 then? I'm just wondering how I should approach this, because at the moment, having to completely copy and paste this entire method (and the method below because it's protected) into a module (just to change something ever so slightly) is just nasty.

@tractorcow
Copy link
Contributor

For 3.1 or 3.2, subclass SearchForm and put the overridden method in your subclass, then use DI to replace it. :) You just have to do it in user code, I'm afraid.

@nglasl
Copy link
Author

nglasl commented Jan 13, 2016

Alrighty, I was hoping to avoid that, but it looks as though it may need to be the case. I like the idea of this going back into master, however I would much rather a revised solution where MySQLDatabase doesn't return a paginated list immediately, so it gives some flexibility over what the search form ends up with (allowing an extension hook). I'll go back to the drawing board for now, but thank you very much for your immediate responses. It really is much appreciated :)

@nglasl nglasl closed this Jan 13, 2016
@tractorcow
Copy link
Contributor

I'd rather completely replace the searchEngine() method, personally. I'd like the search method to be broken out into a separate injectable dependency in the Database controller, similar to how other objects like DBConnector are injected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants