ViewsJoin ignores operator in configuration

Not sure it I'd call this a bug, feels more like a feature (with a tiny bit of documentation already existing), but whatever.

No reason this shouldn't work as far as I can tell. This would need a fair amount of tests.

I am guessing that as far as not breaking existing behavior, the existing test coverage suffices

We can but hope :) But it comes back green, so lets assume it doesn't break anything (and if it does, we just uncovered a hole in our test coverage).

Adding coverage to \Drupal\Tests\views\Kernel\Plugin\JoinTest sounds like a perfect place to start. Either just add to testBasePlugin() or if you feel there is too much other test config in the way from previous assertions, add a new test method and start from there. Add tests for different operators and see what happens.

Also we need to make really sure to run the tests on all supported databases (just queue some more tests after you upload a patch), because edge case stuff like this tends to break across different databases. I tried some other operators then = on Postgres and that seemed fine, but we have to be sure.

Updates to patch as per#6.

Tests are inline with existing coverage for these settings and look good.

+++ b/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
@@ -269,7 +269,8 @@ public function buildJoin($select_query, $table, $view_query) {
-    $condition = "$left_field = $table[alias].$this->field";
+    $operator = !empty($this->configuration['operator']) ? $this->configuration['operator'] : '=';
+    $condition = "$left_field $operator $table[alias].$this->field";

Should we be worried about existing views with operators other than = now behaving very differently? I.e. do we need an upgrade path to change any operators to = so as to not have a behaviour change?

Could this be expanded to support the 'formula' operator that can be used in SQL conditions? This would be useful in referencing different tables to the left of the join. Right now I have to use 'formul' in a WHERE condition which is less efficient.

Reroll

Re #11 : As far as I can tell, this configuration would come from ViewsData and not Views config, so not sure how we could do an upgrade path for that. But might be missing something.

But it might be worth it to add a CR that if somebody had defined an 'operator', it will now actually get used.

+++ b/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
@@ -310,7 +310,8 @@ public function buildJoin($select_query, $table, $view_query) {
+    $operator = !empty($this->configuration['operator']) ? $this->configuration['operator'] : '=';

Instead of falling back to a default here, wouldn't it be better to set this in the constructor where we are already setting some default values? That way we keep the default settings together.
So add it to this:

    // Merge in some default values.
    $configuration += [
      'type' => 'LEFT',
      'extra_operator' => 'AND',
    ];

#23. Updated the patch as requested.

Added a CR but needs instructions for what action to take, which I am not sure about.

Should a new example be added in JoinPluginBase?

+++ b/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
@@ -258,6 +258,7 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
     $configuration += [
...
+      'operator' => !empty($this->configuration['operator']) ? $this->configuration['operator'] : '=',

This doesn't need the ? because it is getting added with +=, which in effect already does the empty check. So less logic needed, which is good I think :)

CR looks great, added a short line about the possible steps you need to take to update your code.

#25. Yes, of course. Should be fixed in this patch,

Glad you liked the CR.

But it might be worth it to add a CR that if somebody had defined an 'operator', it will now actually get used.

Yes this is my first thought too. We've had broken code for a long time, that might suddenly start working and cause an unintended consequence.

For that I think we should add a CR.

@larowlan, do you mean the existing CR needs more information?

Sorry, I missed that there was already a change notice

+++ b/core/modules/views/src/Plugin/views/join/JoinPluginBase.php
@@ -258,6 +258,7 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
+      'operator' => $this->configuration['operator'] ?? '=',

We don't even need the ??, it can just be

'operator' => '=',

right?

@Lendude, sorry to keep taking your time.

Thanks for taking the time to keep coming back to this :)

Nothing to add from me anymore, looks ready.

Committed and pushed 1760fab770 to 10.0.x and 2ce5ee3db5 to 9.4.x. Thanks!

Given the CR only merged back to 9.4.x