Views InOperator::getValueOptions() children do not return values like InOperator::getValueOptions()

Looks like a testable bug to me...

Historically this function was not meant to return something, but yeah I always hated that design.

These are the examples we should change in order to be consistent.

• \Drupal\views\Plugin\views\filter\LanguageFilter::getValueOptions
  

• \Drupal\user\Plugin\views\filter\Roles::getValueOptions
• \Drupal\user\Plugin\views\filter\Permissions::getValueOptions
• \Drupal\user\Plugin\views\filter\Name::getValueOptions
• \Drupal\taxonomy\Plugin\views\filter\TaxonomyIndexTid::getValueOptions
• \Drupal\file\Plugin\views\filter\Status::getValueOptions
• \Drupal\comment\Plugin\views\filter\NodeComment::getValueOptions

Making sure that the other plugins all have proper return values.

1. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -89,7 +89,9 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
   +  public function getValueOptions() { /* don't overwrite the value options */
   +    return $this->valueOptions;
   

   This should be converted to a // comment on it's own line in between. Also capitalize the D and put a . at the end

2. +++ b/core/modules/user/src/Plugin/views/filter/Name.php
   @@ -102,7 +102,9 @@ protected function valueSubmit($form, FormStateInterface $form_state) {
      // Override to do nothing.
   

   Hmm. This and the other one should probably be the same comment. Maybe it needs rewriting in general. Also they could use {@inheritdoc} docblocks.

Thanks @tim.plunkett . I made the above changes and attached a new patch.

@ChuChuNaKu,
Your changes look good, but based on the files suggested for change by @dawehner, you missed making changes to:
\Drupal\user\Plugin\views\filter\Permissions::getValueOptions
\Drupal\views\Plugin\views\filter\LanguageFilter::getValueOptions

Should these also be changed?

Also in some of the cases there are comments that note: // Don't overwrite the value options.
Should there be a comment such as //Overwrite the value options.
or something similar in functions that do change them?

YEAH IMHO we should change all of them.

Sorry about that! I included the two missing changes.

+++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
@@ -89,7 +89,13 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
+    // Don't overwrite the value options.

+++ b/core/modules/user/src/Plugin/views/filter/Name.php
@@ -101,8 +101,13 @@ protected function valueSubmit($form, FormStateInterface $form_state) {
+    // Don't overwrite the value options.

This is a super weird comment now. Can we maybe come up with something better?

I just used the comment that was already there but I'm open for suggestions. How about $this->valueOptions should not be overwritten ?

So the two methods where the comment is debated the methods override the base implementation in InOperator. That has:

  public function getValueOptions() {
    if (isset($this->valueOptions)) {
      return $this->valueOptions;
    }

    if (isset($this->definition['options callback']) && is_callable($this->definition['options callback'])) {
      if (isset($this->definition['options arguments']) && is_array($this->definition['options arguments'])) {
        $this->valueOptions = call_user_func_array($this->definition['options callback'], $this->definition['options arguments']);
      }
      else {
        $this->valueOptions = call_user_func($this->definition['options callback']);
      }
    }
    else {
      $this->valueOptions = array(t('Yes'), $this->t('No'));
    }

    return $this->valueOptions;
  }

(Note the InOperator already returns the options :P) So in effect, what we are doing in this patch is we return the value even if not set. (With or without this patch, options callback is not supported). Not sure this can be succinctly explained in a short comment. I guess Yes/No are not good defaults for a taxonomy term or username :) We could say "return the value options as-is" but that is what the code does, so not a very valuable comment. I would just do away with the comment.

As for what tests are there for getValueOptions():

core/modules/user/src/Tests/Views/HandlerFilterPermissionTest.php:    $view->filter['permission']->getValueOptions();
core/modules/user/src/Tests/Views/HandlerFilterPermissionTest.php:    $value_options = $view->filter['permission']->getValueOptions();
core/modules/views/src/Tests/Entity/FilterEntityBundleTest.php:    $this->assertIdentical($view->filter['type']->getValueOptions(), $expected);

Are all the test mentions.

Also as to whether this is an API change InOperator::getValueOptions() is not defined on an interface. Parents do not implement it. The first definition is on InOperator. That returns the value options. So the children do it wrong to not return it. Not sure if adding an interface would constitute an API change in a funny turn of events given a new requirement :D So for a point release fix, I don't think we should make an interface.

Test coverage. In detail:

- language filter tested with all options
- permissions filter test updated to not call the get method twice, so expecting it to return the right value right away
- roles test updated to assert for roles options
- user name filter test updated to assert NULL (there is nothing that sets the value options, so that will be NULL)

Reviews welcome :)

Great cleanup!
For those methods it is perfect to return effectively NULL, as they don't provide all possible value options. Examples like the taxonomy one would cause too many items at once,
so we rather rely on an autocomplete widget.

+++ b/core/modules/user/src/Tests/Views/HandlerFilterPermissionTest.php
@@ -74,7 +74,6 @@ public function testFilterPermission() {
-    $view->filter['permission']->getValueOptions();
 
     // Test the value options.
     $value_options = $view->filter['permission']->getValueOptions();

Nice small cleanup!

@dawehner: yeah, re the one line removed, now that the permissions are returned in all cases immediately, we can/should get rid of calling it twice :)

  /**
   * Child classes should be used to override this function and set the
   * 'value options', unless 'options callback' is defined as a valid function
   * or static public method to generate these values.
   *
   * This can use a guard to be used to reduce database hits as much as
   * possible.
   *
   * @return
   *   Return the stored values in $this->valueOptions if someone expects it.
   */
  public function getValueOptions() {

Let's fix up the @return documentation as we're now saying that it should always return $this->valueOptions so the "if someone expects it." seems a bit weird.

I'm not sure but maybe also it might be worth a CR for existing contrib views plugins to find out about this fix as they might have copied the incorrect behaviour - from one of the child classes.

Updated with that suggestion.

Change record draft at #2633182: Views filter InOperator::getValueOptions() children should always return the stored value options.

Thank you

Committed 66ae877 and pushed to 8.0.x and 8.1.x. Thanks!

+++ b/core/modules/views/src/Plugin/views/filter/InOperator.php
@@ -59,8 +59,8 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
-   * @return
-   *   Return the stored values in $this->valueOptions if someone expects it.
+   * @return array|NULL
+   *   The stored values from $this->valueOptions.

I've committed this to 8.0.x because even though it is an API change now everything conforms to the documentation.

published the cr