Limiting options for exposed Language filters causes errors and doesn't work for special languages

The patch looks good to me
and works as expected.

Nice find!

+++ b/core/modules/views/src/Plugin/views/filter/LanguageFilter.php
@@ -67,7 +67,10 @@ public function getValueOptions() {
+      $this->valueOptions = array_map(function ($value) { return (string) $value; }, $this->listLanguages(LanguageInterface::STATE_ALL | LanguageInterface::STATE_SITE_DEFAULT | PluginBase::INCLUDE_NEGOTIATED, array_keys($this->value)));

I like what this does but this line is pretty unreadable, maybe wrap it at some point?

Also, this needs tests.

As a bug this can go against 8.1.x

Worked for me

Worked for me too, thanks.

This patch works for 8.2.3 too.

#2 fix the notices on 8.2.3

Patch #2 still applies for 8.3.x and solves the issue for Search API based views, especially Search API Solr Multilingual.
I increase the priority because this issue heavily spams the watchdog on every single search.

I think the better choice would be to adapt reduceValueOptions to account for TranslatableMarkup objects.

Maybe something like changing

elseif (is_object($option)) {

to

elseif (is_object($option) && !($option instanceof MarkupInterface)) {

or something like that? That should do the trick.

patch #2 solve the notice problem but apply with an offset on 8.4

patching file core/modules/views/src/Plugin/views/filter/LanguageFilter.php
Hunk #1 succeeded at 62 (offset -5 lines).

Thanks CalebD for working on this. I'm unassigning this to open it up for others to work on it.

I'm at the TCDrupal sprint and working on following the meta instructions for major issue triage (https://www.drupal.org/node/2474049).

I was able to reproduce this on simplytest.me with 8.4.0-dev version - see attached image for errors screenshot (errors-8.4.0-dev.png). Then I applied the patch from CalebD which did not produce any errors.

The steps I used to reproduce with and without the patch:

1. Install Drupal 8.4.0-dev with the standard profile
2. Create a view called Test View at admin/structure/views (no other settings changed)
3. Add 'Translation Language' under Context Filtering
4. On the options page for 'Translation Language', check 'Expose this filter to visitors'
5. Also check 'Limit list to selected items' and save the filter
6. Then navigate to the Recent log messages at admin/reports/dblog to see the reported errors

I was searching for duplicates and found a plan around fixing multilingual views issues which this issue seems to fall under so I am linking it.

While looking over the issue to see if the summary is up-to-date, as called for in the major issue triage documentation, I feel like it is. But, as it's likely related to a greater issue with multilingual views, which was linked in comment #17, there could be other issues and it would be good for someone else to review it.

I finished going through all the steps on the meta for major issue triage.

#2 fix the notices on 8.3.4

I had this error on Drupal 8.3.5:

Notice: Undefined property: Drupal\Core\StringTranslation\TranslatableMarkup::$option in Drupal\views\Plugin\views\filter\InOperator->reduceValueOptions() (line 272 of core\modules\views\src\Plugin\views\filter\InOperator.php).

I applied patch #2 (via composer) and it fixed the problem.

Reroll against 8.5.x, applies well to 8.4.x, too.
Taking #5 and #13 into account.

Manually tested #23 and it works fine for removing the warning when the language is used in an exposed filter.

Patch #23 works for me as well. No more log errors.

And it applies cleanly to current 8.4.x-dev and 8.5.x-dev.

marking RTBC

Thanks for working on this and for confirming that it resolves the issue.

We do still need to add an automated test that fails without the fix and passes when combined with it. Thanks!

Patch #23 fixed the issue for me on 8.x-4.x-dev

small notice when applying #23

patching file core/modules/views/src/Plugin/views/filter/InOperator.php
Hunk #2 succeeded at 272 (offset 2 lines).
patching file core/modules/views/src/Plugin/views/filter/LanguageFilter.php

I applied patch #23 on Drupal 8.3.7.

Patch is applied successfully and errors disappear!

I don't have enough internal knowledge of views to write a full on Functional test that attempts to trigger the error naturally, however if the agreement is that LanguageFilter::getValueOptions should not return an instance of MarkupInterface as an element of its array, I'm happy to write that test.

With a failed and passing test, I'm moving this back to needs review.

Looks good to me.
+1 RTBC

Created:
24 Mar 2016 at 18:51 EET
Updated:
20 Feb 2018 at 23:20 EET

2 years for acepting a patch for fixing a notice! ;)

RTBC++ tests added #26

An attempt to apply patch #31 to d8.5.0 at simplytest.me fails with http 500 error.

Do we need this patch re-roll for d8.5.0 ?

I don't think so. I rebased locally against a new 8.5.x pull and it applied fine, but I'll requeue the tests here and will post a new patch if they fail.

The tests pass against 8.5-dev and 8.6-dev. This should be fine to commit to 8.6 and cherry-pick for 8.5.1

1. +++ b/core/modules/views/src/Plugin/views/filter/InOperator.php
   @@ -271,7 +272,7 @@ public function reduceValueOptions($input = NULL) {
   +      elseif (is_object($option) && !($option instanceof MarkupInterface)) {
   

   Is this change still necessary when it's being changed in LanguageFilter? Also it doesn't look like the test coverage handles this code.

2. +++ b/core/modules/views/tests/src/Kernel/Handler/LanguageFilterTest.php
   @@ -0,0 +1,33 @@
   +    foreach($values as $key => $value) {
   

   Very minor - missing space after foreach().

Tested patch #31 again at simplytest.me and it applies cleanly now.

It could be that simplytest.me platform had some issues these days...

Strictly speaking it isn't necessary to check twice. Either reduceValueOptions should gracefully handle markup, or we should require filters to return strings.

Either reduceValueOptions should gracefully handle markup, or we should require filters to return strings.

Since requiring filters to return strings would be an API change, that would be harder to do. Currently the return value is just array.

   * @return array|null
   *   The stored values from $this->valueOptions.

Also it can't require just strings, it also has support for objects, as long as they have certain properties set....

Handling this in reduceValueOptions sounds like the better route then. Probably just an
elseif ($option instanceof MarkupInterface) {$option = (string) $option; }
or something along those lines?

#31 fixes array_shift warning when using language filter with limited options.
--Drupal 8.5.3

thanks everyone

Thanks! #31 fixes this error for me on 8.5.4 when limiting language options:

Warning: array_shift() expects parameter 1 to be array, null given in Drupal\views\Plugin\views\filter\InOperator->reduceValueOptions()

Patch from #31 works for me too as mentioned in #42 and #43.
-- Drupal Core 8.6.x-dev

#41 and the reporter are correct, in_operator reduceValueOptions should handle translatable markup. The author of #31 clearly had no idea what he was doing. In fact, if you follow back through FilterPluginBase::buildExposedForm() The filter system will handle TranslatableMarkup in options just fine, all InOperator needs to do is recognize the difference between a markup object and an options object, and pass the markup through like it would a string.

Here's a new test showing that in_operator can handle translatable markup in options, and a much cleaner fix. No interdiff because it's a from scratch new approach.

Would be great to get this in

#46 mikelutz:

The author of #31 clearly had no idea what he was doing.

#31 mikelutz

LOL :D

Credited @Lendude, @tstoeckler, @catch for review comments

Committed and pushed 0b6c398e7b to 8.7.x and 6cd36113bf to 8.6.x. Thanks!