Remove duplicate condition

After looking at the code for a while, I can't see any reason why this check should be there.

It was added when the views was merged into core and hasn't changed since. Soon after the merge there was a commit for changing variable names to comply with coding standards (camel-case), but for unknown reason the property field_alias in FieldPluginBase wasn't changed (strange):

This

if (!empty($field->field_alias) && !empty($field->field_alias)) {
  $fields[] = "$field->table_alias.$field->real_field";
}

Became

if (!empty($field->field_alias) && !empty($field->field_alias)) {
  $fields[] = "$field->tableAlias.$field->realField";
}

But the if condition(that has the duplicate condition) didn't change since.

The combine filter query basically searches on fields that do have non-empty field_alias property. I've looked at the FieldPluginBase and found that the field_alias is defaulted to 'unknown'. Also looked at places where the field_alias could be set to anything that would evaluate to empty, but it seems to me that it always is not empty.
=> So if I am right the condition in the if will in every situation evaluate to TRUE.

Another strange thing is that every field is selectable when configuring the combine filter. The Combine::buildOptionsForm doesn't do this check. So the question would be why is the Combine::query doing the check => inconsistency.

I am uploading this patch (removing the conditionally field add) for testing.

Side note:

One big problem here is the lack of documentation(like for the field_alias), many violations of drupal 8 coding standards, unstructured code. I really think that the core views needs refactoring. Like for example Sql has 50+ phpcs warnings/errors. Also interfaces are wrongly implemented, many methods can hardly be followed.

Okay, this failing is actually a good thing. The bad thing is that for me locally the failed tests don't fail. They actually do 0 passes 0 fails and 0 exceptions, which is strange. I will keep trying to make it fail.

So I forgot to put the ViewAjaxTest test in. This passes successfully, but all the others do 0 passes. Still these tests for me all appear to be passing, but something still goes wrong, as sometimes I get redirected to the test list from the result page and also noticed the AJAX error showing in between of test running page and result page. Might this be an issue whit testing (ran with the testing module)??

What is really strange is that all errors are related to JS/AJAX. I can't see how taking out the if connect to these problems :\. Would be nice if I could get it fail locally as I could much more easily track the problem..

As I still can't get these tests to fail, I'll try to go with consistency and check for the same thing that is checked when building the options field and when validating (clickSortable).

Maybe unrelated, but without change or with these changes when I do the tests I get a page where AJAX response is printed out between the test page and the result page. I can access it by running the test than going back one in the browser:

An AJAX HTTP error occurred.
HTTP Result Code: 200
Debugging information follows.
Path: /batch?id=8&op=do_nojs&op=do
StatusText: OK
ResponseText: 
Welcome to Drupal Dev | Drupal Dev
Skip to main content
Toolbar items
Back to site
Manage

...

I don't have an answer, but I might be able to point someone in the right direction.

I tried running this locally and got the same failures that the testbot did; I noticed some error messages in my apache log files:

[Mon Oct 24 17:16:06.943125 2016] [negotiation:error] [pid 18526] [client 127.0.0.1:55974] AH00687: Negotiation: discovered file(s) matching request: /var/www/drupal-issues/2810985/views (None could be negotiated)., referer: http://localhost/drupal-issues/2810985/test-glossary

If I disable MultiViews (in my apache config), the tests pass fine. I have always had it on and have never had problems with running Drupal tests before.

Seems extremely unlikely, but it might be something to investigate.

Haha - I think the problem is actually the name of the patch - resubmitting the patch from #9 under a different name (if I understand correctly, #13 was a best-guess attempt at fixing the test errors, and #9 is the fix we actually want?). I think the "views." at the beginning of the file is confusing apache when multiviews is enabled (and it looks like it probably is on the testbot - http://cgit.drupalcode.org/drupalci_testbot/tree/containers/base/web-bas...)

Thank you @vaplas for confirming that it doesn't fail for you aswell.
@csheltonlcm I wouldn't have thought about that. You just saved me from spending even more time figuring out why the tests don't fail locally.

What is left to do is to decide whether we keep the patch #9 without the condition or the patch #13 with the clickSortable condition. I think we should most likely go with clickSortable, because it states in the Combine::buildOptionsForm that a field that doesn't have this might break the filter (going to investigate more on why tough).

$options = array();
foreach ($this->view->display_handler->getHandlers('field') as $name => $field) {
   // Only allow clickSortable fields. Fields without clickSorting will
   // probably break in the Combine filter.
   if ($field->clickSortable()) {
       $options[$name] = $field->adminLabel(TRUE);
   }
}

But it's arguable as the user can only select these type of fields.
I am re-uploading the #13 patch just in case.

Why they would "probably break" is a key question on keeping that clickSortable() and needs a comment around why.

Closed #2882841: Combine fields filter includes a duplicate condition in if() statement as a duplicate of this. I like the patch we have there better, because in the if() we are actually testing for the variables we are using inside the if().

Background on the clickSortable usage in Combine: #2401953: Database exception for View with Combined field filter with fields with no query alias, short answer: it's waiting for #2349465: Add an isComputed method to field handlers to land.

Reposting the proposed patch from #2882841: Combine fields filter includes a duplicate condition in if() statement

As far I can tell, this should be RTBC, unless those conditions are always true.

The patch from comment #24 is for me the correct solution (RTBC). We still need testing for this issue.

Test patch #24 on 8.8.x-dev.
Patch is apply clearly, this is ready for RTBC .

This issue needs automated tests.

This patch will fix the issue.

@ shubham.prakash: Your patch is a copy fom the patch from comment #24.

This issue needs automated tests.

Reverted the issue back to before the edit in #36 which deleted all the info in the IS.

Kindly review patch for version 9.5x,,

Moving back to needs work for the test cases

Patch #44 tested and applied successfully for drupal 10.1.x version.

Added patch against #44 in Drupal 10.1.x

Patch #44 still applies for 10.1 so #49 was not needed. Updating credit.

Lets see if this works.

test added and the fix is trivial

Committed/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x, thanks!

I unchecked some commit credit where the patch uploaded was identical to the previous patch (and the previous patch still applied).