- Issue created by @ressa
- 🇩🇰Denmark ressa Copenhagen
This should probably be a feature request, since it is not a bug, and the module works well for normal use.
- 🇩🇰Denmark ressa Copenhagen
Moving to Search API module, since that is probably a more correct place in the process to address this?
- 🇩🇰Denmark ressa Copenhagen
Moving back to Leaflet, since I discovered the
$fallbackHandler
function in Search API. - 🇩🇰Denmark ressa Copenhagen
Specify that
geofield_default
("Raw Output") is the FieldFormatter in play. - 🇮🇹Italy itamair
Thanks for opening this Ressa.
But it would be pretty complicate for me to setup and reproduce your use case for me ...
both because I don't have time to dedicate to this, also not real scenario to apply and also not so familiar with Solr (can you believe I really never setup it up locally, etc. ...?).Would you (be able to) go on, on this, and provide direct contribution ... such as
proper debugging and fix of this, throughout a (strictly not regressive and PHP/Drupal coding standard compliant) patch or Merge Request ...? - 🇩🇰Denmark ressa Copenhagen
I understand @itamair, there are many components in play here ... Sadly, my coding skills are too limited to update the code.
If you're interested, I could create a very simple example for you, and include the steps to set up and import it into a DDEV/Solr instance? Setting up Solr in DDEV is super easy: https://github.com/ddev/ddev-drupal9-solr/
Alternatively, if you don't have time right now, do you think it's possible to use a simple hook in a custom module to specify that
geofield_default
("Raw Output") should be used as FieldFormatter? - 🇮🇹Italy itamair
Well of course it would help if you create an example to look at, or to clone and reproduce locally with DDEV.
If you help me reproducing your use case I could look into it and better understand if the simple hook or whatever could be the most appropriate solution ... - 🇩🇰Denmark ressa Copenhagen
Thanks, that sounds good. I tried to replicate the problem, and the other day it worked as expected, and I couldn't replicate it. But then I did the same steps (maybe with other field names?) and the issue was back...
So here are are Composer files and database dump of a very basic site for import into DDEV, containing two nodes with polygons stored in Solr, and a Views map. Let me know if anything is unclear about setting it up.
- 🇩🇰Denmark ressa Copenhagen
Also, the title might not be correct, since it seems like sometimes, the polygons do show with "Use entity field rendering" disabled, so fallback might actually work ... but I won't update it, since I am not sure.
- 🇩🇰Denmark ressa Copenhagen
I have updated the modules, and added a step to update Solr configuration in DDEV, which I might have forgotten in the first set up from July 2023. Sorry about that. I am attaching a fresh tar.gz-file with updated Composer files, database dump, etc. to allow restoring a local example Drupal 10 install with Leaflet and Solr in DDEV.
I finally got Xdebug breakpoints working in PhpStorm:
- https://www.drupal.org/docs/develop/development-tools/editors-and-ides/c... →
- https://ddev.readthedocs.io/en/latest/users/debugging-profiling/step-deb...
I found the place in the code where the problem originates, by adding a breakpoint at line 891 in /modules/leaflet_views/src/Plugin/views/style/LeafletMap.php:
$this->moduleHandler->alter('leaflet_map_view_geofield_value', $geofield_value, $map, $leaflet_view_geofield_value_alter_context);
Both with or without "Use entity field rendering" enabled, this is the Views row result, which is correct:
$this->rowTokens ( [0] => Array ( [{{ field_geoarea }}] => Drupal\Core\Render\Markup Object ( [string:protected] => MULTIPOLYGON (((9.387817 56.122081, 8.783569 55.789959, 9.113159 55.542101, 9.981079 55.486117, 10.090942 56.073058, 9.387817 56.122081)), ((9.761353 55.355176, 8.997803 55.429013, 8.794556 55.148535, 9.761353 55.355176)))
The resulting value in
$geofield_value
looks like it is the problem:-
With "Use entity field rendering" the returned value is correct
MULTIPOLYGON (((9.387817 56.122081, 8.783569 55.789959, 9.113159 55.542101, 9.981079 55.486117, 10.090942 56.073058, 9.387817 56.122081)), ((9.761353 55.355176, 8.997803 55.429013, 8.794556 55.148535, 9.761353 55.355176)))
-
Without "Use entity field rendering", directly from Solr, it doesn't work
POINT( 8.783569 55.789959 MULTIPOLYGON (((9.387817 56.122081)
It looks like it messes up the format, and mistakenly tries to use the second value as a point, ending up with malformatted value in the end.
PS. drupal.org adds an underscore in the file name, so you need to rename it:
- From leaflet-view-solr-2023-9-9.tar_.gz
- To leaflet-view-solr-2023-9-9.tar.gz
- 🇩🇰Denmark ressa Copenhagen
Adding screendumps from PhpStorm.
Breakpoint and Views row value
With Entity rendering, correct value
Without Entity rendering, wrong value returned
- 🇩🇰Denmark ressa Copenhagen
It actually looks like the problem is this function, starting at line 312 in /modules/leaflet_views/src/Plugin/views/style/LeafletMap.php:
public function getFieldValue($index, $field) { $result = $this->view->result[$index]; if ($result instanceof SearchApiResultRow) { $search_api_field = $result->_item->getField($field, FALSE); if ($search_api_field !== NULL) { $values = $search_api_field->getValues(); } if (!empty($values)) { foreach ($values as $key => $value) { [$lat, $lon] = explode(',', $value); $values[$key] = sprintf('POINT(%s %s)', $lon, $lat); } return $values; } }
If the "Use entity field rendering" is enabled, the Geofield value skips the
[$lat, $lon] = explode
part and is in the correct format ("MULTIPOLYGON (((12.7342 55.7032,12.7339 55.7027,12.7334 [...]
") and the multipolygons are rendered correctly.With "Use entity field rendering" disabled, the value is exploded into
$lat
$lon
values, like this:$lat = "MULTIPOLYGON (((12.7342 55.7032" $lon = "12.7339 55.7027"
@itamair: Should we reach out to @drunken monkey and ask for help?
- 🇹🇷Turkey orkut murat yılmaz Istanbul
Hello all,
Actually, new version of Search API → solved a lot of problem of mine. Have you tried it with this issue too?
Best,
Orkut - 🇩🇰Denmark ressa Copenhagen
Thanks for the suggestion @Orkut Murat Yılmaz, sadly the problem is still there ...
If you are interested in taking a look at this problem, you're more than welcome to check out the Solr example (from 9 September 2023) with Composer files, database dump, etc. to allow restoring a local example Drupal 10 install with Leaflet and Solr in DDEV.
- 🇮🇹Italy itamair
Hey Ressa ... sorry for my lack of participation here (but I am much engaged also in some other work stuff, unfortunately not related at all with Drupal Mapping).
I am going to jump on this asap (hopefully in this weekend) and indeed try to reproduce your use case, by using what you provided (also for setting SOLR locally with DDEV).Will try to come back here with some detailed feedback and possibly also some solutions clues.
- 🇩🇰Denmark ressa Copenhagen
Hi @itamair, no problem, I totally understand. We all have to prioritize, and paid work comes first, since it is needed to allow voluntary work.
About Getting Paid for Open Source Work, you could consider, as Webform → does, to offer "Professional support" or "Fund development" on Geofield and Leaflet via Open Collective.
Anyway, I am very grateful for your efforts with maintaining so many map-related modules in Drupal.
- 🇮🇹Italy itamair
@ressa thanks a lot for providing such a solid and detailed testing env on this, with DDEV, Solr and Geofield content already there.
All the configuration and installation process was fine and I was properly able to reproduce and review your use case.The good news are that it indeed looks something on the Leaflet module side, so we should be able to fix this autonomously.
Could you check the attached patch and properly test / QA and let us know if it solves your issue here? (as I hope ...)
BTW: don't you have any account in the Drupal Slack channel (drupal.slack.com)?
I wanted to live chat with you, on this ... so as on my beloved Denmark
(I lived in your super cool country for 3 splendid years! and I still miss it pretty much) - Status changed to Needs review
about 1 year ago 11:00pm 26 October 2023 - 🇮🇹Italy itamair
slight (and better) update of the previous patch.
Please focus and QA/Review this one ... - Status changed to Active
about 1 year ago 6:24pm 27 October 2023 - 🇩🇰Denmark ressa Copenhagen
Thanks a lot @itamair for looking at this. I am glad that the set up worked well for you, it seemed like the simplest way to share what is a slightly complicated environment.
The patch works great, I can disable "Use entity field rendering" and the Leaflet view is now rendered just as expected, as when "Use entity field rendering" is enabled.
I am not sure how to tell if Solr or MySQL is getting the requests, though ... I tried disabling caching with this:
drush state:set twig_debug 0 --input-format=integer && drush state:set twig_cache_disable 0 --input-format=integer && drush state:set disable_rendered_output_cache_bins 0 --input-format=integer
... and then reload the page a few times, by clicking "Admin Toolbar Extra Tools > Flush Views cache", with Webprofiler enabled. These are the results (Solr="Use entity field rendering" disabled):
MySQL: ~250 requests, ~100 ms Solr: ~220 requests, ~75 ms
So there is an improvement, which is great, but do you know if there is a better method to test this? If not, it's fine and I will change status to RTBC, since the error has been fixed by you.
I don't use Slack, sorry. I am glad you enjoyed your time up here in the cold North, where it's now fridge temperature, at 5°C and time for the Long Johns :-) Where did you live?
- Status changed to Fixed
about 1 year ago 7:51pm 27 October 2023 - 🇮🇹Italy itamair
Hi Ressa ... no, I don't have much experience with Search API and SOLR, and their fine testing.
SOooo ... let's close this as fixed and add this improvement in a brand new Leaflet 10.2.3 release, as all this really looks a well done enhancement to Drupal Leaflet.
PS: I lived 3 great years in the wonderful CPH, working as Senior BE (Drupal) developer for Unity Technologies ...
- 🇩🇰Denmark ressa Copenhagen
All right, since the performance does look good, it can wait until and if it becomes an issue. I tried the latest release, and it works perfectly. Unity, nice -- they became big!
Automatically closed - issue fixed for 2 weeks with no activity.
- Status changed to Fixed
about 1 year ago 9:22am 29 November 2023 - 🇳🇱Netherlands Lendude Amsterdam
We updated to 10.2.12 and one map displaying ~600 pins stopped loading, giving out-of-memory errors where previously it was loading fine. Tracked it down to this change. Something here makes the caching go crazy and loopy until
\Drupal\Core\Cache\CacheTagsChecksumTrait::calculateChecksum
runs out of memory. Not sure if this is the fault of core caching, search API caching or how this patch changed things but here is a patch reverting this change for people running into similar problems and don't need this functionality - 🇩🇰Denmark ressa Copenhagen
Thanks for sharing a patch @Lendude -- perhaps an issue should be created, aimed at keeping Geometries support, while fixing the caching issue?
- Status changed to Needs review
10 months ago 5:35pm 28 March 2024 - 🇮🇹Italy itamair
Thanks @lendude ...
@ressa, well of course the patch #32 looks reverting all that was done here, so it is not advisable in order to keep Geometries support.
But in my Leaflet playground / testing env I am experiencing much simpler code (keeping the processing simplicity required by @lendude) still able to provide support to Geometries in Search API Leaflet view, and with the option "Use entity field rendering" unchecked.
@resa could you test and QA the new attached patch, that performs a good amount of code simplification, if it is still able to render Geometries in your use case? and also @lendude eventually ...
- Status changed to Needs work
10 months ago 7:17pm 28 March 2024 - 🇩🇰Denmark ressa Copenhagen
Thanks for a fast patch @itamair, sadly it doesn't work for my map ... I don't get any errors, but the same two nodes are shown, regardless of there being no facets, or after updating facets. I'll try with the attached leaflet-view-solr-2023-9-9.tar_.gz as well.
- 🇩🇰Denmark ressa Copenhagen
I have now verified with the example from #14 🐛 Add Geometries support for Search API Solr and Views integration Fixed , and it also doesn't work with the patch.
If you follow the steps in the leaflet-view-solr-2023-9-9.tar_.gz README, you should be able to have a working example running in a few minutes. The only thing you need to do is:
- Update to the latest release of Leaflet (10.2.12), and see that multipolygon values are shown
- Update to Leaflet dev-version, apply the #34 patch, and see that it stops working
DDEV is such an amazing tool.
- Status changed to Needs review
10 months ago 2:40pm 29 March 2024 - 🇮🇹Italy itamair
@ressa ... yes, I rebuilt your nice example (provided with the great DDEV) and was able to properly QA & Review patch #34,
and yes indeed we cannot get rid of the specific code under that checks if$result instanceof SearchApiResultRow
(otherwise we would completely loose search API rendering)BUT it looks that we could further & greatly refactor & simplify (less redundant) the same code in the 10.2.x dev branch at the moment ...
Could you please re-QA and Review the new attached, that looks working fine to me, both in a Search API / Solr Leaflet View and in a normal Leaflet View?
Thaaaanks!
- 🇩🇰Denmark ressa Copenhagen
I am glad to report the patch works well, and that the Geofield multipolygon values are rendered fine, and just the same way as with the latest Leaflet release. Also, it renders fine in a regular View as well. Thanks @itamair!
@Lendude: Perhaps you can check if the out-of-memory error is solved with the patch?
- 🇳🇱Netherlands Lendude Amsterdam
Applied the patch in #37, unfortunately still runs out of memory. No time to test further I'm afraid.
- Status changed to Fixed
9 months ago 7:51pm 2 April 2024 - 🇮🇹Italy itamair
Thanks @ressa ... patch #37 is a code improvement and has been committed into 10.2.x branch. It will be part of the next Leaflet module 10.2.x release.
Sorry for your issues #Lendude ... may be you will need to re-roll your patch, to roll back.
But it looks you might be experiencing issues depending by your specific setup.
May be you should provide more extend information on your uses case, how to eventually reproduce it.
And still I am not sure if you are experiencing your out of memory on a normal Leaflet Map View or on a Search API based one Automatically closed - issue fixed for 2 weeks with no activity.
- 🇧🇬Bulgaria i.koychev Sofia
Hi #Lendude
We have the similar issue when we try to display a list of pins on the leaflet map using Search API, Geofield, Leaflet & Views (the issue is 2 years old Drupal 9/10).
So far we haven't found a solution.
When the pins are more than 800 (we have 1300) the view reports out of memory.
The coordinates in search api index are defined as type of string.
We use Search API Indexed based View with the following settings:
- Format: Leaflet Map
- Data source: coordinates (field)
- Simple Tooltip: title (field)
- Leaflet popup: Decription (field)
- Leaflet Map Tiles Layer: OSM Mapink
- Enable Leaflet Markercluster Js Library functionality - On.
Fields:
- Coordinates
- Title
- Decription
The issue occurs only when we use Search API Index based View.
I'm not sure which module this issue should be addressed to. - 🇳🇱Netherlands ekes
Just a heads-up if anyone else notices this. I've not had time to investigate myself but we have a report that this patch broke a Search API based view https://github.com/localgovdrupal/localgov_directories/issues/378
- 🇳🇱Netherlands ekes
> The issue occurs only when we use Search API Index based View.
Regarding the memory usage, as I see it here. I've looked at in the past, and did supply a patch or two that helped bring it down. But what I noted was:
First with Search API either DB or Solr unless you have done quite some work you will be doing an entity load for every row returned (so every point). You can do the work with Solr to make it use the field data and not do an entity load. This will reduce your memory usage significantly. There is a patch in Search API module I started to do the same with the db backend, but it's only a start, and requires much more work.
The second point where there is quite some memory usage is the same if you use views sql or search api, and probably doesn't come into play if you're talking just 1000 points or so. But I'll mention it here as I looked at this too at the same time. When constructing the points for display on the map there is a lot of configuration and the whole loop over all the points and creation of the output is quite intensive. This I don't see any way round, because doing anything else would loose the features of the module, being able to configure the output such. With flexibility comes some complexity.
- 🇮🇹Italy itamair
Agree with @ekes.
This issue changes properly enable Geometries support for Search API Solr and Views integration.
And it is also true that Leaflet View style render method perform some intensive tasks to generate "features" that are then sent to the js layer for FE rendering with the leaflet.sj library itself.
Every new logic that could better streamline all those tasks, without breaking any (present of backward) functionality || removing their associated options/properties (that further FE Map alterations could rely upon) are really welcome.
For instance, in most recent commits I added token / replacements patterns support for Tooltip and Popup options, for leveraging dynamic use of those (based on features properties) ... those are new cool features, that of course come with additional calculation complexity.But, worth to mention that (in my actual experience) this complexity needs to be processed only the first time, if proper FE caching is enabled, and after that Leaflet Maps loads really fast, also with hundreds of markers.
Proper tuning of Marekclustering also makes a lot of difference (and performance improvement) in this context. - 🇬🇧United Kingdom grahamC Oxford, UK
To elaborate on the #44 issue - the regression we're seeing in Localgov Drupal after this change is because it no longer falls back to the default Views getValue() if it *is* a SearchApiResultRow but doesn't have a usable corresponding field.
I don't have the why on this part, but on this view the $real_geofield_name comes back with `location` but there's no such field in the result row. (There is a `localgov_location` field, but that returns a simple lat/long rather than a POINT(), so doesn't work here either...)
The attached patch adds the fallback behaviour back in and fixes the issue for me.
- 🇳🇱Netherlands ekes
> don't have the why on this part,
I can investigate further if absolutely necessary. But I think it is going to be because: (a) as mentioned, even if you index a field it is not used in the results from Search API (except in very specific Solr configurations); (b) the values that are used are from the loaded entity; (c) this does not yet include related entities, and in this case the geofield is on an entity referenced from the base entity.
- 🇮🇹Italy itamair
@grahamC I am adding here a new incremental patch that applies to actual 10.2.16,
that should still retain the added Geometries support for Search API Solr and Views integration,
and eventually fallback return values coming from normal View in such uses cases as yours (that I cannot exactly reproduce on my side).
Could you test and QA that, and check if it correctly fix your reported scenario? - 🇬🇧United Kingdom grahamC Oxford, UK
@itamair Yes! Tested on the site we originally saw the problem on, and new patch #49 resolves the issue.
- 🇮🇹Italy itamair
Ok ... patch #49 committed into 10.2.x dev branch, is going to be part of the next incoming 10.2.17 release.
- 🇳🇱Netherlands idebr
Out of memory issue reported in #32 still occurs with Leaflet 10.2.20 using a Search API view. Attached patch fixes it.
@idebr Confirmed your patch fixes the OOM issue and about doubled the performance of a page with a large map. Thanks!
@itamair Would we be able to get the patch in #52 committed? I'd be happy to make an MR for it if that helps.
- 🇮🇹Italy itamair
Thanks @idebr! Pretty cool your patch is still preserving Geometries support for Search API Solr and Views integration,
and confident it could solve/mitigate also the out of memory issue ...
I committed it into the 10.2.x-dev branch, with an additional Note of mine but still setting you as Author of the that,
I just deployed a new drupal leaflet 10.2.21 release → with that ...
Great Job! (hope it really solves the out of memory drama) - 🇳🇱Netherlands ekes
> Out of memory issue reported in #32 still occurs with Leaflet 10.2.20 using a Search API view. Attached patch fixes it.
So far as I can see this stops loading the field data from the Index Item
Item::fieldsExtracted
is basically only set onceItem::getFields
has retrieved them from the backend. So unless something else is calling this before leaflet does, the data will be falling back to load it from the entity. Which is what some use cases are trying to avoid. This will usually work, and you won't notice unless putting break points in. It does not work for some other cases, where the field is on a referenced entity.If I'm correct that @idebr case means, either something else has added the extracted data (not search_api or search_api_solr code that I see), or loading it (probably also retrieving it from the entity) within
Item::getFields
takes more memory than directly doing agetValue
on the field is probably a Search API issue, not one for Leaflet to solve.