We should add "Ensure that destination is valid"-condition exact to the same condition.
So I added new condition group and added our conditions to it.

We couldn't add another parameter to opSimple() method because of

Warning: Declaration of Drupal\rg_location\Plugin\views\filter\GeofieldContainsFilter::opSimple($options, Drupal\Core\Database\Query\Condition $condition) should be compatible with
Drupal\geofield\Plugin\views\filter\GeofieldProximityFilter::opSimple($options)

So I rename method to opSimpleMethod().

Patch will be attached.

CommentFileSizeAuthor
#13 geofield-proximity_filter_and_condition-3110969-13.patch788 bytesitamair
#11 screenshot.jpg247.02 KBsuper_romeo
#4 3110969-4.geofield.Proximity-filter-wrong-condition-operator-if-OR-between-view-filters.patch3.36 KBsuper_romeo
#3 3110969-3.geofield.Proximity-filter-wrong-condition-operator-if-OR-between-view-filters.patch3.29 KBsuper_romeo
#2 3110969-2.geofield.Proximity-filter-wrong-condition-operator-if-OR-between-view-filters.patch3.21 KBsuper_romeo

Issue fork geofield-3110969

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

super_romeo created an issue. See original summary.

super_romeo’s picture

super_romeo commented
Status: Active » Needs review
StatusFileSize
new 3110969-2.geofield.Proximity-filter-wrong-condition-operator-if-OR-between-view-filters.patch3.21 KB
super_romeo’s picture

super_romeo commented
StatusFileSize
new 3110969-3.geofield.Proximity-filter-wrong-condition-operator-if-OR-between-view-filters.patch3.29 KB
super_romeo’s picture

super_romeo commented
StatusFileSize
new 3110969-4.geofield.Proximity-filter-wrong-condition-operator-if-OR-between-view-filters.patch3.36 KB
super_romeo’s picture

super_romeo commented
itamair’s picture

itamair commented
Status: Needs review » Closed (works as designed)

I don't see any need for this. The "Ensure that destination is valid" is already added to $this->query ...

super_romeo’s picture

super_romeo commented
Status: Closed (works as designed) » Needs review

Sorry, but it's really important.
Conditions added with this filter should be in it's own condition group.

Consider I want to add isPublished filter.

Compare:
Wrong:

isNotNull($lat)
&& isNotNull($lng)
|| isPublished

and
Correct:

(isNotNull($lat)
&& isNotNull($lng))
|| isPublished

It is like 2 + 2 * 2 and (2 + 2) * 2.

super_romeo opened merge request !2

super_romeo’s picture

super_romeo commented

Rerolled.

itamair’s picture

itamair commented
Status: Needs review » Postponed (maintainer needs more info)

@super_romeo I don't really understand your code, that is quite far to pass my code review.
It looks mostly un-effective to me. It seems you are refactoring some existing code, without improving it.
For instance at its end you are adding this:

$condition->where(geofield_haversine($options) . ' ' . $this->operator . ' ' . $this->value['value']);

that is already added in the $this->query just the code row before.

And I also don't like you are adding and using this new 'opSimpleMethod' that is not present in the parent 'NumericFilter'.

But more than that I don' really understand which is the bug that you are trying to solve (may be my bad).

What is the bad outcome of what you call: "wrong condition operator if OR between view filters"?

Please, you clearly need to explain with details (and may be with @screehsots too) as I need really to understand, how to reproduce the use case that is not working for you with the actual code, and have evidence how your patch is solving it ...

Till I will assume that everything is working as expected, on this proximity filter side.

super_romeo’s picture

super_romeo commented
StatusFileSize
new screenshot.jpg247.02 KB

This is the view with OR between filters:
screenshot

This produces following SQL:

SELECT "node_field_data"."created" AS "node_field_data_created",
"node_field_data"."nid" AS "nid",
"node_field_data"."langcode" AS "node_field_data_langcode"
FROM {node_field_data} "node_field_data"
LEFT JOIN {node__field_geo} "node__field_geo"
ON node_field_data.nid = node__field_geo.entity_id AND node__field_geo.deleted = '0'
WHERE ((( 6371 * ACOS( LEAST(1, COS( RADIANS(1)) * COS( RADIANS("node__field_geo"."field_geo_lat") ) * COS( RADIANS("node__field_geo"."field_geo_lon") - RADIANS(1) ) + SIN( RADIANS(1) ) * SIN( RADIANS("node__field_geo"."field_geo_lat") ) ) ) ) < 1000))
OR (("node__field_geo"."field_geo_lat" IS NOT NULL) AND ("node__field_geo"."field_geo_lon" IS NOT NULL))
OR ("node_field_data"."type" IN ('test'))
ORDER BY "node_field_data_created" DESC
LIMIT 11
OFFSET 0

Take a look at OR (("node__field_geo"."field_geo_lat" IS NOT NULL) AND (...
OR should be AND.

itamair’s picture

itamair commented
Title: Proximity filter: wrong condition operator if OR between view filters » Proximity filter: correct AND condition on valid destination coordinates
itamair’s picture

itamair commented
Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new geofield-proximity_filter_and_condition-3110969-13.patch788 bytes

Ah ok, Thanks ... now I got what you meant. Nice catch!
The much simpler attached patch is fixing this issue for me, in different filter combinations and groups.
Please review and report if you find any issue or regression with it.
I am going to commit shortly otherwise, and deploy a new Geofield 8.x release with it ...

PS: Let's keep an eye also to the Drupal patch naming conventions.

itamair’s picture

itamair commented

  • itamair committed 4082ea0 on 8.x-1.x 
    Issue #3110969 by super_romeo, itamair: Proximity filter: correct AND...
itamair’s picture

itamair commented
Status: Needs review » Fixed

Further tested. Wrking fine. Committing into dev and new module release.

super_romeo’s picture

super_romeo commented

You are welcome :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

itamair closed merge request !2