🇮🇹Italy @itamair

Thanks! New 8.x-1.59 release deployed with this fix!

itamair → made their first commit to this issue’s fork.

I would be ok with this MR !54 (and merge it)
BUT I don't get why dependency injection was removed from the src/Form/SettingsForm.php
with all those changes that now would rely on:

$typedConfigManager = \Drupal::service('config.typed');

I don't think this is required by Drupal 11,
and would revert all that, unless I am provide with clear motivation for that.

Thanks @here!

itamair → made their first commit to this issue’s fork.

Ok. QAing and Reviewing this now ...

ok ... thanks @jan kellermann,
not sure how this could be useful, as the Leaflet Js core library doesn't support perspective (may it would with adds-on libraries?),
but indeed the MapLibreGL JS does, so may be Drupal Leaflet Views / Maps only with Tiles (and not loaded Features) might benefit from this ... (is't it?).
Going to merge this now into the 10.2.x dev, after just having reduced redundancy in leaflet.api.php documentation
This will be part of the next Drupal Leaflet 10.2.19 release ...

itamair → made their first commit to this issue’s fork.

Thanks. This was merged and is going to be part of the next Leaflet 10.2.18 incoming release.

Don’t worry @jonathan1055
It’s all good here.
Let’s not make any drama of this simple task.
The 2 removed files were just un-used assets files, and you already mentioned them, so we are all good.
Lol … and let’s enjoy life 😉

Well ... I didn't read any feedback to my latest update.
I am feeling my last update / commit is pretty solid, hence I am going to merge this into 8.x-1.x dev, for being part of new upcoming release.

Actually the Geofield value field is generated in the MySql Db as a LONGBLOB ...
So I don't think this is a real issue. Could we close this?

Ok ... thanks! Let's merge also this one, into 8.x-1.x dev

itamair → made their first commit to this issue’s fork.

itamair → made their first commit to this issue’s fork.

Actually you have been the only one credited for this one, and also author of the commit (and such a small commit also ... just 3 / commas) ;-)

Thanks. Merging this into the 8.x-1.x dev, will be part of the next Geofield 8.x-1.58 release.

Ok. MR!31 merged into 8.x-1.x dev, will be part of next Geofield 8.x-1.58 release ...
Thanks a lot for the great Job!

ok ... thanks @here!
I am going to sit for final green flag for merging this Geofield Drupal 11 compatibility Issue:
https://www.drupal.org/project/geofield/issues/3430742 📌 Automated Drupal 11 compatibility fixes for geofield RTBC
and then will proceed with this one also ... :-)

Hi @here!
and sorry if I couldn't sync and stay on track with this ... BIT I had other priorities.
I could see some improving commits till some days ago, even after this was tagged as RTBC.

I wouldn't really re-review and QA all this, as I can already see many strong Drupal devs on this.
Hence, are we sufficiently confident in merging this MR into the 8.x-1.x dev branch and deploy a new geofield 8.x-1.58 release, compatible with Drupal 11???

Hi @Diana, thanks for your support request.

To accomplish your task you should consider that both the Leaflet Map View style and the Leaflet Formatter have the "Icon URL" option that support Token and Replacement Patterns,
so you could use inside that some dynamic values that change depending on some property (field value) of the entity that you want to render, and that (for instance) generate a dynamic url pointing to the specific Marker Icon Url you want to associate to the specific entity/feature being processed in the map.
It could be a specific field/property value of the entity, or the field/property value of a specific taxonomy term that is attribute to the entity, etc.
Hope a couple of attached screenshot could make what I explained even more clear ...

Feel free to reopen this if you still need clarification and support.

Ok ... patch #49 committed into 10.2.x dev branch, is going to be part of the next incoming 10.2.17 release.

Soo @ekes ...
I better inspected all this, and I ended up with the conviction that is still nice to define a GeofieldItem::isEmpty() method that not only make sure that the geofield primitive/row (WKT) input is not empty but also that it generates a valid /Geometry.
That is correct, and we should keep it, and also the Test code base that assumes it ...

What we should rather limit, as you correctly mentioned, is the redundant generation of the same /Geometry in the same GeofieldItem class, to limit the time/memory script overhead and better apply the DRY principle.

That's why in my latest commit a preferred to keep basically unchanged the GeofieldItem::isEmpty() method and rather to define a private \Geometry $geometry property that could be generated once and reused also in the GeofieldItem::populateComputedValues() method.

Tests are still passing nicely with this change and $geo_php_wrapper->load($value) is only called once in the class.

I would commit this that looks a nice improvement to me.
Let me know if you disagree, if you see some issues or potential regressions, or evident mistake with this solution of mine.

thanks @ressa,
and also @jErry_jakcson ...

Running composer require --dev drupal/core-dev solved the problem for me too.

thanks @ekes for your updates. Going to re-jump and review your last comment asap ...

No, I don't think (pretty sure) the elevation data is picked / extracted from the GPX file, and for sure I know it is not stored and managed by the Geofield field type, that basically matches (is based upon) the WKT format (https://libgeos.org/specifications/wkt/) that indeed doesn't support elevation info into it.
May be you could implement your own hook / extension (of the Geocoder GPX plugin processor) that also extract the z - elevation data and somehow stores it somewhere in your destination entity ...

hi Rafa ... it looks that looking/googling for GPX and Geofield (Leaflet dependency) & Geocoder you will find this relevant Documentation exactly on what you ask here: https://www.drupal.org/docs/extending-drupal/contributed-modules/contrib... →

Isn't it?

(Feel free to reopen if my question is not so exhaustive, as I feel).

Please look for existing previously reported (and longstanding tasks) before assuming and creating new Bug tasks,
that cannot be ... (you should ask how you could be the only one reporting this, out of thousands & thousands of adoptions).

@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?

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.

Hi @calmforce and thanks for reporting this,
BUT I tested what you reported and all looks fine for me.
I cannot reproduce the Bug and the regression that you describe ... also applying your exact use case.

As you can see from the attached screenshots, my same local Leaflet View that reflects this Leaflet Demo one here:
https://www.geodemocracy.com/drupal_geofield_stack_demo/web/geoplaces-ma...

is correctly rendering circle markers that reflect the following options (and so on ... ):

{"radius":5,"color":"red","fillColor":"#f03","fillOpacity":0.5}

So eventually you just need to re-set / re-save your Leaflet View settings ...

It is true the latest 10.2.14 version made some changes in the Leaflet Circle Marker definition that are set and send to the js layer for final rendering, such the following:

https://git.drupalcode.org/project/leaflet/-/commit/b3f99b37dc933cb0610c...

but those would affect only you functional logic IF you are setting them in a hook_leaflet_views_feature_alter hook,
and YES in that case you should adjust your code and alter the

icon.circle_marker_options value

rather than the previous

marker.icon.options

Closing this as cannot be reproduced ...

(feel free to reopen if you really find evidence of this BUG and where I was clearly wrong in my review and statements)

Thanks @ressa for all this 🙏

Thanks again ... merging this.

Thanks for this ... but I also want to properly QA and Review this.
Not clear to me if this fix is needed only in the Leaflet Formatter,
or something needs also to be adjusted / fixed in the Leaflet View Style also
(usually they should pair each other ... )

itamair → made their first commit to this issue’s fork.

Please have a look to this parent issue that I just created.

https://www.drupal.org/project/geofield/issues/3444247 💬 Create a route or trip photo reportage with Geofield Stack (with gpx and exif data) Fixed

You should find your way beyond that ... (or rather go for sponsored support).

itamair → created an issue.

Happy you did it with Geolocation …
And pretty weird because Geofield is also very good in accomplishing that, in very different and powerful ways.

Look, for instance, this MtBike trip photographic reportage, with all geo mapping, of course with Geofield stack:

https://www.geodemocracy.com/via-degli-dei-2024/web/

I will post soon a thread with more details and foundation hint on that 😉

itamair → made their first commit to this issue’s fork.

#2 patch worked for me also. Let's deploy it in a new module release?

Of course you cannot represent a Map only with the Geofield module.
And you need either the Leaflet drupal → module or the Geofield Map → one,
to generate a Geofield based Map View that is using the geofield and is able to show different Contents (nodes or whatever entities) of mixed types/bundles,
and eventually storing either single POINTs or Routes (from GPX), and even other Geometries (such Polygons), etc ...
Worth to mention that those views will work very fine also with Exposed Filters based on your Contents / Entities properties (such as type of POI, or Route type/taxonomy, etc.).

But, once you got how to populate a geofield and make a Drupal (Leaflet or Geofield Map) view around it, everything else relates with your Drupal further skills and creativity (so goes beyond the supporting scope of this issue, imho).

Last thing: why are trying to use Geofield when it looks that you are able to accomplish all your tasks with the Geolocation module?

Also this new documentation page looks come right in time and purpose on this:
https://www.drupal.org/docs/extending-drupal/contributed-modules/contrib... →

@effortDee it stand under meant that you cannot represent and manage the gpx file / route in the Geofield field, but of course you need to transform you gpx file / content into geofield format (WKT) and that is possible with the Geocoder module → and its GPX geocoder provider.

If you don't know how to do that, well you could get your competences and your way starting from this relevant community comment from the past:
https://www.drupal.org/project/geocoder/issues/2747193#comment-11829776 →

Last (but not least). Please don't mix the geofield naming with the geolocation, as this latter could refer to the geolocation module → , that is different from (and not supposed to be mixed with) the geofield module one.

well I think I did as I checked both of you just before triggering the Merge command, and your usernames appear (as by) in the commits messages, and your one as author of the commits (both on the 3.x and 4.x branches)
I don't know what I should do more than that ...

Right you are @ressa, as (almost) always 🙂

I am closing this afternoon days without any review to my support.

there could be different ways to accomplish what you desire ... and in a more correct than the one you are trying to implement,
and for different Drupal competence ranks.

For instance one possible that I would suggest could be the following:

- use one content type for instance called "Route" where to store a line string route (generated geocoding a GPX file, as in your use case) into a geofield type field called for instance: field_geofield;
add a taxonomy reference field, called "field_route" where to store (or reference) the name of the specific Route (yes you will generate or need to generate a relevant Routes vocabulary);
- use one content type for instance called "POI" where to store single Points of Interest into the same geofield still called field_geofield (yes ... you are going to re-use the same geofield of the "Route" content type. Also in this content type add a taxonomy reference field, called "field_route" where you are going to store (or reference) the name of the specific Route (or Routes, because it could be the same POI could be relevant to more than one Route, isn't it?). Also add another taxonomy reference field, called "field_poy_type" that could tag / characterise each POI with a specific "POI Types" vocabulary ...
- after having accomplished the above 2 tasks, start populating your Routes and POIs, populating each of them field_route with the relevant Route vocabulary term(s). PS: yes, you will have each Route content named and tagged (on the with "field_route" value) probably with the same route name ...

- generate a Geofield Map View (could be a Leaflet View with the leaflet ,module, or a Google Maps with the Geofield Map, etc.) based on the field_geofield and filtered on the specific "field_route" you want to represent (this could also be accomplished with an exposed filter, and quickly switch to different Routes POIs froth same View) ...
- and that's it ... in few (and not even too few words);
----
The described above could just be one possible scenario ... but according to your Drupal skills you could also accomplish more refined and powerful ones.
For instance also a paragraphs types and contents and paragraphs functional logos could be accomplished (given that Geofield fully supports also paragraphs ... ).

Pretty confident I provided you with valuable inputs ... I wish you good luck and happy Geofield Drupal mapping!

thanks ... committed, with some improvements, will be part of the next Leaflet module 10.2.x release

itamair → made their first commit to this issue’s fork.

thanks @ressa ... great contribution, as usual (so far)

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

thanks for this @ressa ... I am going to review this asap ;-)

@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!

Ah! got it @m.stenta ...
Thanks for the detailed explanation. Sometime I loose detailed track of what it is going on all over these Geofield stack of modules.

What you say makes perfect sense, but let's keep it as example @MigrateProcessPlugin and not harmful placeholder for Geofield.

I provided this new commit:

https://git.drupalcode.org/project/geofield/-/commit/483958ae703f02f46cb...

where I simply added all your warning into the @MigrateProcessPlugin description itself.

Hi @m.stenta … I cannot (I am not sure I can) get what you mean in “deprecating” it …
The geofield.wkt_generator service still is used (neede) elsewhere.

What is further needed on this? (also with an additional issue …?)

thanks @m.stenta
definitely right you are ...
geofield.wkt_generator service is needed in the geofield_latlon @MigrateProcessPlugin but not in the geofield_wkt one.

The following (just performed) commit accomplishes what you suggest in the 8.x-1.x branch:
https://git.drupalcode.org/project/geofield/-/commit/77e70c1b0ba74478984...

It will be part of the next Geofield 8.x release.

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 ...

Thanks for reporting this and your contribution ...
BUT all that you describe is not happening.

I reproduced your use case (that is very common for the Drupal Geocoder module, and it thousands of adoptions)
and all is working fine on my playground and testing envs,
with Drupal 10 and 8.x-4.23 Geocoder release, and lates Address module (2.0.1) ...
(none of the errors that you report here).

Thus I am changing this into Support Request and to Minor priority,
and closing this as "cannot reproduce", as indeed that's the case..

Bear also in mind that your actual MR is probably making useless changes ($dumper_result) should always be a string because of the fixDumperFieldIncompatibility method definition) and (more bad) is introducing bad Drupal & PHP Coding standards issues (look the indentations ... ).

All your issues should depend from some specifici (or wrong) configurations of your specific use case.

PLEASE, don't change this back into Bug report, unless you could give clear evidence that you spotted a VERY General and reproducible use case / issue for every user ...

Regards

Thanks for reporting this,
BUT the GLOB_BRACE is an official parameter for the PHP glob() function.

Users experiencing this issue should rather switch to version of their PHP docker container that properly support that ...
rather than this Drupal module introduce a new dependency from a third party PHP library (that in your MR is not even properly required by the internal module composer.json).

Closing this as "works as designed".

Please always inspect if there is already an issue addressing your same one, before opening a new one.
Such in this case would be this one: https://www.drupal.org/project/geofield_map/issues/3423274 ✨ Gmaps not loaded with async Fixed

Thanks for reporting this @waako ...
This doesn't look a big deal to me (but rather a kind of picky one), also because this doesn't happen on a general basis,
but ok ... it makes some sense to me, and I agree to merge your MR.
This will be part of the next Leaflet module release.

itamair → made their first commit to this issue’s fork.

Thanks @ressa ... your contribution is always super valuable and pertinent.
I fixed your lates remark in the lates commit, that will be part of the next Leaflet 10.2.x release.

Here under is the description of the security issue and correspondent workaround suggested here: https://security.drupal.org/node/180274

This module has a potentially malicious 3rd party JS vulnerability.

You can see this vulnerability by:
1. Enabling the module
2. polyfill.min.js is included via the polyfill.io domain.

"As of February 24, 2024, cdn.polyfill.io, the domain hosting the polyfill.io JavaScript library, has been acquired by a Chinese company named Funnull. Polyfill.io is a widely used JavaScript library integrated into many of the world's most well known web applications. All polyfill.io traffic is now pointing to the Baishan Cloud CDN (https://www.baishancloud.com/)."

https://polykill.io/
https://github.com/polyfillpolyfill/polyfill-service/issues/2834

Both Fastly and CloudFlare have provided temporary fixes for this issue
https://blog.cloudflare.com/polyfill-io-now-available-on-cdnjs-reduce-yo...
https://polyfill-fastly.io/

Latest Leaflet 10.2.12 release → indeed (already) fixed this security issue,
changing the reference to the Polyfill.io library by pointing to the https://polyfill-fastly.io/ domain
as discussed and suggested here: https://security.drupal.org/node/180274

Please also look the related Advisory for this Leaflet security issue: https://security.drupal.org/node/180276

thanks. Patch #4 committed into dev, will be part of the next Geofield Map release.

You should be able to do that just inspecting how the "leaflet_markecluster" options is being generated in the $js_settings["map"]["settings"] index in a Leaflet View style, basically here:
https://git.drupalcode.org/project/leaflet/-/blob/10.2.x/modules/leaflet...

For instance for this Leaflet View demo map: https://www.geodemocracy.com/drupal_geofield_stack_demo/web/geoplaces-ma...
my xdebug is returning the structure expressed in the attached screenshot.

Thanks a lot @ekes for this new opportune and qualitative contribution.
Merged into 10.2.x dev branch ...

errata corrige ... the patch #13 and the latest geofield map module release 3.0.15,
still don't get rid of this issue warning, hence I am reopening this,
though with "minor" state (as it is ... ).
Don't have a clear fix for this at the moment ...

Will welcome any solid and general fix to this issue, if any incoming.

Patch #12 committed into dev, will be part of the next incoming Geofield Map release.
Thanks ... closing this as Fixed.

Ok. Got all this ...

according to this: https://developers.google.com/maps/documentation/javascript/versions
the v=weekly is the way to go to align with the most current and up-to-date version ...

New patch attached is still fixing this issue to me.

Please clear Drupal & Browser cache after applying.
It removes the warning reported in this issue, in my general use cases (inspect the mentioned demo page above).

It won’t be possible to switch to a general use of google.maps.marker.AdvancedMarkerElement in the context of this issue, because the module still relies on wrapping libraries (such spiderify and markerclusterer that would need refactoring upstream).

Simply having removed the reported warning from Google maps library looks good here.

And please don’t move back to “needs work” unless you clearly reported a negative outcome of the #3 patch.

thanks @ekes, all this looks making sense.
Will properly qa and review asap ---

@john.oltman could you better elaborate why the #3 patch didn't help you?

Please bear in mind that it applies (should be applied) to the actual 3.0.x-dev HEAD (and not to 3.0.14 version).

Actually that patch is being applied to this Geofield Map demo page:
https://www.geodemocracy.com/drupal_geofield_stack_demo/web/

fixing / resolving that warning, as you can see form the web browser console,
how it is being documented in the attached screenshots.

It looks that change is going to require / load latest Google Maps v.3 library version ...
and also accomplishing your suggestion in your latest comment.

Please correctly apply the #3 patch, and clear your drupal & browser cache, and come back again to this review.
Eventually explain what is not working for you, because #3 looks a general fix to this, to me.

Thanks for reporting this.
It looks that more generally requiring a Google Maps API v3 fixes this.
Please Test, QA and Review the attached patch that looks getting rid of that google.maps APIs warning to me,
and if confirmed could be committed into dev and be part of the next Geofield Map module release.

Thanks @sidgrafix ... nice catch and fix.
I slightly changed it, but committed still crediting it to you.
This fix will be part of next Geofield Map module release ...

The "geometry" option was added to this demo Leaflet map: https://www.geodemocracy.com/drupal_geofield_stack_demo/web/geoplaces-ma...
on the Content Type exposed filter options, and all looking good also on that, that is just filtering and extending the Map bounds only to polygons geometries ...

It means to me that all is looking good in the bounding box extension also with geometries (not points).
Re-closing this as "cannot reproduce".

Okay. May be some regression might have been introduced in bounding box on polygons.
Re-opening this ... will better check this out asap.

thanks for reporting this ...
BUT I can see a js console error appearing when I check the "Congressional" option ...

SyntaxError: JSON Parse error: Unrecognized token '/'

It indicates (might give the clue) that something could be wrong in that specific source data of you, or the way it is being parsed by the Drupal process. And that makes sense, as the bounding box is working fine on the "Democratic Club", isn't it?

Also please note that this Leaflet Demo page: https://www.geodemocracy.com/drupal_geofield_stack_demo/web/geoplaces-ma...
is using Leaflet module version 10.2.11 and its bounding boxing is working apparently fine (@see and check the "Content type" exposed filter on top left under the Leaflet View title ... ).

Please properly check on your specific project settings.
I couldn't reproduce your issue on the 10.2.11 version.

This looks to me an exact (and outdated) duplication of this one, already solved: https://www.drupal.org/project/geocoder/issues/3395751 🐛 with Drupal 10.2.xx - PHP Fatal error: Type of Drupal\geocoder\Form\SettingsForm::$typedConfigManager must be typed Fixed
Please let me know if I am wrong, and why ...
or please mark this as Closed (duplicate) if confirmed.

Thanks. Implemented in both in 8.x-4.23 and 8.x-3.47 ...

Thanks. Fixed in 8.x-4.23 and 8.x-3.47 ...

Double checked ... no issues in the lates 8.x-4.22 version on this.

I am reopening this ... but just to quickly double check.
Please move on into Geocoder version 4.22.
Your issues still definitely look related to cache clear ...
Please double check.
The DumperPluginManager Service looks fine in tis dependency injection.

Thanks for catching this. Version 8.x-4.22 (and 8.x-3.45) fixes this issue.

just pushed another commit to fix (hopefully) all phpcs issues and make all pipelines passing

Done!

... did you clear drupal cache after upgrading guys???
As it looks could depend on that to me.
Updated Drupal\geocoder\DumperPluginManager looks fine to me, also in its dependencies injection.

Ok ... good job. I added some commits to fix (or ignore) all Phpstan errors/issues.
Gitlab CI going to be deployed along new Geocoder release (both 4.x and 3.x branch).