The boolean string filter doesn't result in correct where clause

handlerfilterbooleanstring.patch queued for re-testing.

I'm wondering if I dare to just mark this RTBC? It's a fairly obvious fix affecting one line of code, and the patch definitely works for me, and we'll need it on api.drupal.org the next time we update the API module, so that we can make a listing page of all items marked with @deprecated documentation tags. And your Views tests pass.

Yes, I dare to do it! You can always mark it back to "needs review" or "needs work" if you disagree. :)

Is it possible that this could get committed sometime soon?

I would like to deploy some new code from the API module to api.drupal.org sometime soon, and it depends on the fix to this issue. I can grab the dev version of Views, but dealing with a patch in the bzr repo there is kind of annoying... Please? :)

It does look to me as though the Drupal 8 code is probably making the same mistake.

The code is in
core/modules/views/lib/Drupal/views/Plugin/views/filter/BooleanOperatorString.php
and just like the D7 code, it's calling
$this->query->addWhere([group], $where).
on line 40.

However, I'm not sure what this addWhere() method is. It doesn't appear to be part of Views' QueryPluginBase class, which is what HandlerBase::$query is documented to be.

Oh I see. It's on the Sql class.

Yeah, it looks like it should be calling Sql::addWhereExpression() and not Sql::addWhere.

Here's a completely untested patch to fix this. I think it's correct but unlike in D7 I do not have code in place to test it.

So... Shouldn't the QueryInterface class in D8 views have this addWhere method and others on it that SQL has, so that a Handler like Boolean String whatever it's called can call this method reliably? And shouldn't the $query member variable on the Handler base class be documented to be QueryInterface rather than QueryPluginBase? And yes these questions are both off-topic for this issue... but it's sloppy to say the least...

I checked the other filters in the same directory and I am pretty sure their calls to addWhere() are fine. None of them is attempting to pass in a string WHERE clause except this one. I didn't go back to Views 7.x contrib to check there.

I also looked at the arguments directory in D8 core Views and didn't see anything obvious there either.

So this one filter is probably the only place this problem occurs? hopefully.

Well, the test bot didn't complain. I'm not going to RTBC my own patch here because I didn't test it in D8, but the equivalent patch is definitely working in D7.

Well, let's see. I used it in the D7 API module... What I had is a database field with this hook_schema() definition:

      'deprecated' => array(
        'description' => 'Deprecated descriptions',
        'type' => 'text',
        'size' => 'medium',
        'not null' => FALSE,
      ),

This database field stores the description that people put after the @deprecated tag in API documentation.

And what I wanted to do is be able to use that to filter API documentation based on whether that database field was empty or not. So I put the following into hook_views_data():

    'deprecated' => array(
      'title' => t('Deprecated'),
      'help' => t('Whether this item is deprecated or not'),
      'filter' => array(
        'handler' => 'views_handler_filter_boolean_operator_string',
      ),
    ),

And then I was able to use this as a filter in a view, to set up a page displaying all deprecated functions (which is coming soon to api.drupal.org, like maybe today or this week, now that the views 7 patch has been committed):

  $handler->display->display_options['filters']['deprecated']['id'] = 'deprecated';
  $handler->display->display_options['filters']['deprecated']['table'] = 'api_documentation';
  $handler->display->display_options['filters']['deprecated']['field'] = 'deprecated';
  $handler->display->display_options['filters']['deprecated']['value'] = '1';

So this handler basically allows you to set up a filter for a text-containing database field that acts as a Boolean. I'm not sure if it would be useful in Core for anything or not... I cannot think of anything off-hand?

The documentation on the test class needs some work:

a) I think we need @inheritdoc on the getInfo() function?

b)

+   * Tests the boolean filter with grouped exposed form enabled.
+   */
+  public function testFilterBooleanOperatorString() {

The word Boolean is a proper noun and should be capitalized.

c)

+   * @return array
+   */
+  protected function getGroupedExposedFilters() {

@return statements need a line of descriptive documentation as well.

d) There may be an extra blank line at the end of the test file.

e) It would also probably be good to attach just the test addition part of the patch so we can verify that it fails without the code fix (to verify that it is testing the actual bug reported here).

OK on (a). Stupid getInfo().

Anyway, the docs now look good, and I gave both the code patch and the tests a close review. They make sense to me. The patch is what was needed in Drupal 7 Views to fix this bug, and the tests certainly look like they are testing this exact functionality.

So assuming that the test bot returns red for the FAIL patch and green for the PASS patch, I believe this is good to go, and I'm marking it RTBC. I suppose that when the FAIL patch fails, the bot will change the status, but we can change it back then.

fixing inadvertent tag re-add

Back to RTBC, since the two changes in the latest patch appear to have fixed all the views_get_view() calls.

There was a patch for 7.x in the very early stages of this issue, which is here:
https://drupal.org/files/handlerfilterbooleanstring.patch

On comment #5, @dawehner said he committed it, but it may never have gotten pushed. Could we just get it committed and pushed?