Show map on /locations

[mstenta]

This PR takes the first, simplest, and most impactful step towards Issue #2551091: Show map in /locations and /asset/[id]/locations.

It adds a map of locations to the /locations page in farmOS.

It does NOT add a map of locations to the /asset/[id]/locations pages. This has additional complexity, and I don't think there's a lot of value in taking this first step, and tackling the second part in a follow-up.

This PR also makes another general change, which is to rename/replace the current dashboard map type a locations map type (and also cleans up some dependencies). The thought behind this change is that the current dashboard map type is not dashboard-specific at all (other than the fact that it's only used on the dashboard). It is provided by the farm_ui_map module, not the farm_ui_dashboard module, and it adds layers for all locations to the map. So this just generalizes that to be more reusable, and then uses it in the /location page.

This needs CHANGELOG.md line(s), and will be saved for 3.1.0, so I'll leave it as a draft for right now.

[mstenta]

See this comment for details on what will be necessary for the next step (adding the map to /asset/[id]/locations): https://www.drupal.org/project/farm/issues/2551091#comment-15403543

Tl;dr: the main thing we need is a custom contextual filter for asset parent that we can use in our farm_asset_geojson View.

[symbioquine]

Not sure if it's worth it, but this could be made backwards compatible (in case any other folks modules might be depending on the map type) by keeping the 'dashboard' map type and issuing a deprecation warning from the event subscriber.

[paul121]

This PR also makes another general change, which is to rename/replace the current dashboard map type a locations map type (and also cleans up some dependencies). The thought behind this change is that the current dashboard map type is not dashboard-specific at all (other than the fact that it's only used on the dashboard).

Removing the dashboard map type would be a breaking change. That is the primary way for another module to add custom behaviors to only the dashboard map, it just happens that farmOS core doesn't do anything else to make it dashboard specific (I have a couple custom modules that do modify dashboard map!). There may be some other ways to hack getting a JS map behavior onto only the dashboard page, but I believe this is a use-case we designed this event subscriber + map type for.

It is provided by the farm_ui_map module, not the farm_ui_dashboard module, and it adds layers for all locations to the map.

I think its possible that we intentionally provided this map type in farm_ui_map. The farm_ui_dashboard module is primarily concerned with providing the dashboard framework and does not need to depend on maps itself. But it would be nice if these two modules did not need to depend on each other. We could consider moving the dashboard map type to be an optional config item within either farm_ui_map or farm_ui_dashboard.

So this just generalizes that to be more reusable, and then uses it in the /location page.

I do like the idea of a location map type, especially if it will be used in multiple places. But I think it would be better to encapsulate this logic into a locations map behavior that could be re-used on any map type. This is a little challenging since we need the logic to determine which asset types are "default location" and also add sub-groups for land and structure types. It's possible we realized this way back and just punted on doing the map behavior since we only needed this logic in one place. But I believe this could be accomplished very similarly to how it works now. The map render event subscribers can first check if a map has the locations layer before using the existing logic to configure the map behavior settings as needed (default location assets & any asset sub-type groups).

[mstenta]

this could be made backwards compatible (in case any other folks modules might be depending on the map type) by keeping the 'dashboard' map type and issuing a deprecation warning from the event subscriber.

Removing the dashboard map type would be a breaking change.

Thanks for raising this @symbioquine and @paul121. I agree we should keep the dashboard map type to avoid breaking anyone's customizations.

But I think it would be better to encapsulate this logic into a locations map behavior that could be re-used on any map type.

I like this idea too...

This is a little challenging since we need the logic to determine which asset types are "default location" and also add sub-groups for land and structure types. It's possible we realized this way back and just punted on doing the map behavior since we only needed this logic in one place.

Yep that's the challenge.

But I believe this could be accomplished very similarly to how it works now. The map render event subscribers can first check if a map has the locations layer before using the existing logic to configure the map behavior settings as needed

Just to clarify, did you mean to say "check if a map has the locations behavior"? I think that makes sense. The locations behavior itself (the JavaScript file) may just end up being an empty function though, since all the actual logic is in PHP. That could work, even if it's a bit odd. Its presence would just be used as a trigger for the PHP logic.

We will also need to add a new blank map type (for the /locations page to use)... because right now the only options are default or dashboard. The default map type currently adds a single grey "All locations" layer, which we don't want on /locations. And we shouldn't add the dashboard map to the /locations page IMO (this would have been the easy solution to all of this, and is what prompted me to create a locations map type in the first place).

[mstenta]

OK I refactored my branch to try out the approach described above. Seems to be working.

What do you think @paul121 @symbioquine?

Some notes:

• This adds a new blank map type and a new locations map behavior.
• The blank map type is used on /locations to ensure that no other layers/behaviors get added to it.
• The locations map behavior JS itself is blank. It is only used to trigger the PHP logic.
• Adjusted the farm_ui_map, farm_land, and farm_structure event subscribers to look for the locations behavior instead of the dashboard map type.
• Added the locations behavior to the dashboard map via the farm_map block configuration in farm_ui_map_farm_dashboard_panes(). I also considered adding it to the dashboard map type config entity itself via an update hook, but there isn't a way to edit the behaviors of those entities right now, it seems. I could see arguments both ways for where it's best to add the locations behavior, but figured this is fine for this step. It might be nice to move it to the config entity in a next step, to make that map type more configurable in general (so someone could more easily remove the locations behavior from the dashboard map). That's not a requirement right now though, and this step maintains current behavior I think.
• I found and fixed a bug in our MapBlock::build() method in the process. See details in the commit message.

[mstenta]

The locations map behavior JS itself is blank. It is only used to trigger the PHP logic.

I just pushed two more commits which remove the need for the JS file entirely, by making the library of a behavior optional. If this makes sense, I'll rebase so the first commit comes before the locations behavior commit, and the second commit gets merged into that one.

[paul121]

The blank map type is used on /locations to ensure that no other layers/behaviors get added to it.

Oh, I do think we should have a locations map type! Sorry if that wasn't clear. I think it is is useful to identify maps based on how they are being used and/or the pages they are displayed on. Unless there is good reason to prevent this capability? Presumably someone could want to add additional behaviors to these location maps, maybe satellite layers or weather data that is only useful at a higher level rather than individual asset or editing maps.

I do see how a blank map type could be useful for rendering maps without any additional behaviors. This would just work because the ID blank is not being used or targeted by anything else. But the same is achieved by creating a map type ID that is specific to the desired use-case. In general I think this is what we should recommend instead of a more generic blank map type. A more specific map type provides the flexibility for others to further customize specific maps. If this really is not acceptable the $event->stopPropagation() method could be called to prevent further modification of a given map. If it is a common need maybe we could introduce a flag to the map type or map element config option eg: allowAdditionalBehaviors that would prevent the event from even being triggered.

It might be nice to move it to the config entity in a next step, to make that map type more configurable in general (so someone could more easily remove the locations behavior from the dashboard map).

Nice, I agree this isn't a requirement for the dashboard map, but I do think this would be most proper for a locations map type.

The locations map behavior JS itself is blank. It is only used to trigger the PHP logic.

Ah interesting. I suppose that is OK but wonder if there are useful helpers or abstractions we could build into the JS side of this? Maybe once we try to limit the assets to those with a given parent?

[mstenta]

I just pushed a few more commits, mostly fixups...

Oh, I do think we should have a locations map type! Sorry if that wasn't clear.

Ah gotcha. OK I pushed a fixup commit (to be merged into previous commits before merging this PR) which adds a locations map type in the farm_ui_location module, and uses that in the /locations page instead of blank.

In general I think this is what we should recommend instead of a more generic blank map type.

Agreed. I pushed a commit that reverts the addition of the blank map type, since it is no longer used (I'll cancel these commits out so neither is included in the final merge).

Added the locations behavior to the dashboard map via the farm_map block configuration in farm_ui_map_farm_dashboard_panes(). I also considered adding it to the dashboard map type config entity itself via an update hook, but there isn't a way to edit the behaviors of those entities right now, it seems.

I realized that it is possible to edit the behaviors of map_type config entities via \Drupal::configFactory()->getEditable()... not sure why I forgot that was an option (I was trying to use MapType::load() and edit it that way).

So... I also pushed a fixup commit that saves the new locations behavior directly to the dashboard map type config entity (rather than tacking it on via the farm_map block configuration). This is the right way to do it, and makes it consistent with the way the new locations map type config entity includes the locations behavior. So now, if someone wants to remove the locations behavior from the dashboard map in their farmOS instance, they can just edit that config entity to remove it.

However, in doing so, I found that the logic I added in an earlier commit was only checking for the locations behavior in $event->element['#behaviors'] and not in the map type's configuration. So I added a new MapRenderEvent::getMapBehaviors() method which returns a merged list of behaviors from both places. Then I added another fixup commit to use that method in the event subscribers.

The locations map behavior JS itself is blank. It is only used to trigger the PHP logic.

Ah interesting. I suppose that is OK but wonder if there are useful helpers or abstractions we could build into the JS side of this? Maybe once we try to limit the assets to those with a given parent?

@paul121 I just want to be sure you saw my follow-up commits which remove the need for that blank JS file. Now the library of a behavior is technically optional, which means it's possible to declare a behavior config entity without providing a JS file. This allows a behavior to just have an ID, label, and description, and then PHP logic can be added in an event subscriber just based on the presence of the behavior ID in the map.

[paul121]

These other changes make sense 👍

I realized that it is possible to edit the behaviors of map_type config entities via \Drupal::configFactory()->getEditable()... not sure why I forgot that was an option (I was trying to use MapType::load() and edit it that way).

Interesting. I would have thought MapType::load(), ->set(), ->save() would work. Kinda surprising. Maybe we could look during the dev call?

@paul121 I just want to be sure you saw my follow-up commits which remove the need for that blank JS file. Now the library of a behavior is technically optional, which means it's possible to declare a behavior config entity without providing a JS file. This allows a behavior to just have an ID, label, and description, and then PHP logic can be added in an event subscriber just based on the presence of the behavior ID in the map.

At first it seemed a little weird but I'm seeing how this could be useful :D Sounds good.

[mstenta]

Rebased onto 3.x, merged fixup commits, and changed the update hook to use MapType::load() (per @paul121's previous comment). I tested the update hook again and it's working the same.

[paul121]

Have not tested, but LGTM!