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

Allow multiple fields for value filter #135

Closed
wants to merge 2 commits into from

Conversation

steefaan
Copy link
Contributor

@steefaan steefaan commented Nov 3, 2016

I think it would be nice to provide the possibility to search a value inside multiple fields at once. This way the Value filter acts as Like filter. Code is BC, see the constructor.

@codecov-io
Copy link

Current coverage is 100% (diff: 100%)

Merging #135 into master will not change coverage

@@           master   #135   diff @@
====================================
  Files          10     10          
  Lines         290    298     +8   
  Methods        51     52     +1   
  Messages        0      0          
  Branches        0      0          
====================================
+ Hits          290    298     +8   
  Misses          0      0          
  Partials        0      0          

Powered by Codecov. Last update 79501da...ec83e1c


return $e;
});
if (!empty($expressions)) {
Copy link
Member

Choose a reason for hiding this comment

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

I would leave out !empty()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a copy/paste from Like filter.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah ideally they both remove the not necessary cloaking. Bot no big deal.

@dereuromark
Copy link
Member

Good idea 👍

@@ -1,6 +1,9 @@
<?php
namespace Search\Model\Filter;

use Cake\Database\Expression\QueryExpression;
use Search\Manager;

class Value extends Base
Copy link
Member

Choose a reason for hiding this comment

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

couldn't we make a MultiValue instead, this will otherwise be breaking change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would stay at Value otherwise we have an inconsistency between the usage of Like and Value. Where is the breaking change?

Copy link
Member

Choose a reason for hiding this comment

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

The default config keys have changed named

@steefaan
Copy link
Contributor Author

steefaan commented Nov 7, 2016

What about merging?

@dereuromark
Copy link
Member

As said before I am 👍 on merging this
Is anyone objecting to this?

@ndm2
Copy link
Contributor

ndm2 commented Nov 12, 2016

Explicit multi field tests wouldn't hurt...

@ADmad
Copy link
Member

ADmad commented Nov 12, 2016

Tests and readme update required.

@dereuromark
Copy link
Member

@steefaan Are you able to finish it up or do you want some help here?

{
parent::__construct($name, $manager, $config);

$mode = $this->config('mode');
Copy link
Member

Choose a reason for hiding this comment

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

We could add an inline comment here:

// For backwards compatibility

etc

@dereuromark
Copy link
Member

How can we finish this up? Anyone interested?

ndm2 pushed a commit to ndm2/search that referenced this pull request Apr 25, 2017
@ndm2
Copy link
Contributor

ndm2 commented Apr 25, 2017

I'll finish it up...

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

Successfully merging this pull request may close these issues.

None yet

6 participants