| [05:00:06] | * tool172[m] has quit (Quit: You have been kicked for being idle) |
| [05:00:35] | * AnasHaddad[m] has quit (Quit: You have been kicked for being idle) |
| [13:46:56] | <mstenta[m]> | paul121: I got it working! |
| [13:54:15] | <paul121[m]> | Woo! What was the trick? |
| [13:54:49] | <mstenta[m]> | Setting `'#parents' => $element['parents']` in the `value` element |
| [13:54:58] | <mstenta[m]> | * => $element['#parents']` in |
| [13:55:27] | <mstenta[m]> | That basically tells Form API to treat the value as the element's canonical value |
| [13:55:34] | <mstenta[m]> | Regardless of whether or not `#tree` is ued |
| [13:55:41] | <paul121[m]> | Ah okay I was curious about #parents!!! |
| [13:55:44] | <mstenta[m]> | s/ued/used/ |
| [13:55:46] | <paul121[m]> | Perfect |
| [13:55:50] | <mstenta[m]> | And then a few minor tweaks to `GeofieldWidget` |
| [13:55:59] | <mstenta[m]> | Just to respect the new structure |
| [13:57:42] | <mstenta[m]> | I actually found the ansewer in core's `Radios` element, and corresponding `OptionsWidgetBase` (for Field API) |
| [13:58:04] | <mstenta[m]> | the `radios` element essentially creates a single element with a bunch of single `radio` elements inside it |
| [13:58:55] | <mstenta[m]> | And the `OptionsWidgetBase` uses a custom `#element_validate` to then do a bit more transformation into the format that Field API needs |
| [14:01:12] | <mstenta[m]> | [OptionsWidgetBase](https://git.drupalcode.org/project/drupal/-/blob/dc4e2a8b36c8b812a8a0315...) |
| [14:01:18] | <mstenta[m]> | s/[OptionsWidgetBase](// |
| [14:01:42] | <mstenta[m]> | https://git.drupalcode.org/project/drupal/-/blob/dc4e2a8b36c8b812a8a0315... |
| [14:01:53] | <mstenta[m]> | Notably, now it works regardless of `#tree` |
| [14:02:56] | <paul121[m]> | Wonderful! |
| [14:04:11] | <mstenta[m]> | Will push updated branch shorty... |
| [14:04:33] | <mstenta[m]> | Still need to finish up docs, but if you have a chance to check out this commit and give the rest an overall thumbs up, then I can go ahead and merge when I'm done |
| [14:04:46] | <mstenta[m]> | I feel pretty good about this! :-) |
| [14:05:06] | <mstenta[m]> | Tested parsing a KML file into geofield too |
| [14:05:30] | <mstenta[m]> | (Would be great to get automated tests for those things... but probably won't be able to add that this round) |
| [14:09:33] | <mstenta[m]> | https://github.com/farmOS/farmOS/compare/2.x...mstenta:2.x-map-form-element |
| [14:09:59] | <mstenta[m]> | Here is the relevant commit: https://github.com/farmOS/farmOS/commit/9d817873264215253fe4245766a52579... |
| [14:14:55] | <paul121[m]> | Hmm sorry only on my phone, but I'm surprised to see the $delta reference removed from the GeofieldWidget. It seems like that is an important part of field widgets. We don't have multivalue geometry fields so it's not a big issue there, but makes me curious about this.... |
| [14:14:55] | <paul121[m]> | Did you try adding farm_map_input to a nested array in a quick form? Just to make sure it uses the proper parents? |
| [14:17:16] | <mstenta[m]> | Ah hmm - let me try that |
| [14:17:33] | <mstenta[m]> | And maybe I can try a multi value geometry field too - good point |
| [14:18:10] | <mstenta[m]> | What do you mean by "adding farm_map_input to a nested array in a quick form"? Like put it into a fieldset with #tree = TRUE? |
| [14:18:19] | <paul121[m]> | If it's just multivalue geometry fields that don't work that's not as big of a problem.. maybe not worth blocking |
| [14:19:02] | <mstenta[m]> | Yea, but you're right... and if we can support it we should. Maybe the bigger question is: do multi value geometry fields work BEFORE this change? |
| [14:19:14] | <mstenta[m]> | I never tested it - are you aware of any existing limitations? |
| [14:19:36] | <paul121[m]> | Not sure! |
| [14:21:24] | <mstenta[m]> | > adding farm_map_input to a nested array in a quick form... (full message at https://libera.ems.host/_matrix/media/r0/download/libera.chat/302173ba55...) |
| [14:21:39] | <mstenta[m]> | * > adding farm\_map\_input to a nested array in a quick form... (full message at https://libera.ems.host/_matrix/media/r0/download/libera.chat/4f3bcc96dd...) |
| [14:26:14] | <mstenta[m]> | Hmm. Well multi-value geofields do not work.... But let me try checking out 2.x and testing on that too. |
| [14:26:23] | <mstenta[m]> | Still - I think you're right that `$delta` should stay |
| [14:26:42] | <mstenta[m]> | (basically just remove the `value` bit, but leave `[$field_name, $delta]` in both places |
| [14:26:54] | <mstenta[m]> | i'll test that on a single value field as well |
| [14:28:11] | <mstenta[m]> | works (with `$delta` as described) on single-value field |
| [14:28:21] | <mstenta[m]> | so maybe multivalue geofields are just broken... testing on 2.x now... |
| [14:28:38] | <mstenta[m]> | if so, i'll open a separate bug report for that and we can figure it out later |
| [14:29:03] | <paul121[m]> | Cool. Yeah not as worried about that |
| [14:29:05] | <paul121[m]> | I gtg! |
| [14:29:11] | <mstenta[m]> | Cool thanks for the eyes! |
| [14:29:34] | <mstenta[m]> | If it's alright with you paul121 I'll merge this after these bits |
| [14:31:14] | <paul121[m]> | Oh.. are we deleting both geofield and geofield_widget? 😁 |
| [14:31:33] | <mstenta[m]> | No just `geofield_widget` |
| [14:31:44] | <paul121[m]> | Oh nvm! Renaming the geofield behavior. Great |
| [14:32:04] | <mstenta[m]> | https://github.com/farmOS/farmOS/commit/a1a015cc580b3d64e9e1c6087d9c389c... |
| [14:32:11] | <symbioquine[m]> | mstenta[m]: I'm not sure "multivalue geofields" make much sense for single records in the farmOS data model... |
| [14:32:50] | <symbioquine[m]> | There's definitely a use-case for something like that though in quick forms and similar where additional data can be associated with each geometry. |
| [14:32:59] | <paul121[m]> | mstenta[m]: Yep |
| [14:32:59] | <mstenta[m]> | symbioquine: I agree. And we will probably never use them. But if the widget we provide is broken on multi-value fields it means NO ONE can use it. |
| [14:33:02] | <mstenta[m]> | So low priority... but still technically a bug. |
| [14:33:15] | <mstenta[m]> | Ah yes and in quick forms it works! |
| [14:33:27] | <mstenta[m]> | Just Field API that seems to be broken at the moment... testing 2.x branch now... |
| [14:33:53] | <symbioquine[m]> | mstenta[m]: Ah, I might not have been understanding the case you were talking about then... |
| [14:34:17] | <mstenta[m]> | AH DANGIT lol - multivalue geofields work in 2.x 🤦 |
| [14:34:31] | <mstenta[m]> | So this actually breaks them :-( |
| [14:34:35] | <mstenta[m]> | Oh well... another thing to debug then... |
| [14:35:40] | <mstenta[m]> | Yea sorry symbioquine - there are two contexts: Form API (eg quick forms, other custom forms, etc) and Field API (entity forms) - it's the Field API mutivalue geofields that aren't working. |
| [14:35:55] | <mstenta[m]> | And we don't currently use "multivalue" geofields anywhere in the farmOS data model - nor should we ever IMO |
| [14:36:09] | <mstenta[m]> | But someone may want to add a Geofield to their own entity and use the farmOS map widget |
| [14:36:32] | <mstenta[m]> | ("multivalue" == "Allow unlimited values" where you can click "Add another" and another map shows up to draw on) |
| [14:37:01] | <symbioquine[m]> | Makes sense |
| [14:37:30] | <symbioquine[m]> | but it would only break if they set the cardinality to >1 or unlimited right? |
| [14:37:34] | <mstenta[m]> | Yep |
| [14:38:02] | <mstenta[m]> | Which... I just tested, and technically that does work currently (in our 2.x branch), but does not work in this new branch... so I should debug it |
| [14:38:45] | <mstenta[m]> | Probably something simple... |
| [14:39:43] | <symbioquine[m]> | worst case could be using a `hook_form_alter` to throw an error that says multi-value Geofields aren't supported in farmOS and suggest that a single geofield that produces a geometry collection might be sufficient... |
| [14:40:40] | <symbioquine[m]> | Little hacky, but at least it would head-off other folks troubleshooting while it's still an outstanding bug. |
| [15:01:17] | <mstenta[m]> | OK! Got it working. :-) |
| [15:02:20] | <mstenta[m]> | Actually backtracked a little bit to the way we were trying to do this originally... using the validate method of the element (NOT the geofield) to set the value. And NOT using `#parents`... |
| [15:03:49] | <mstenta[m]> | `#parents` was the cause of the issue in multivalue geofields... because FormAPI was automatically adding another sub-element for `_weight` and that was then getting used for the value instead of `value` (not exactly sure I understand why... but that's what was happening) |
| [15:07:19] | <mstenta[m]> | ahhh wait... maybe not |
| [15:07:20] | <mstenta[m]> | oy |
| [15:18:00] | <mstenta[m]> | Huh. OK. Well I did get it working... drove me a little nuts for a second there... |
| [15:18:21] | <mstenta[m]> | I'm not sure I love how it's working. But it is working. |
| [15:21:28] | <mstenta[m]> | I'm going to need to give this some more thought.... |
| [15:35:02] | <mstenta[m]> | Eh... well... here's a new commit for review if you have time/interest paul121 |
| [15:35:23] | <mstenta[m]> | https://github.com/mstenta/farmOS/commit/e70b092c0590dccb1914fbe092b653a... |
| [15:36:45] | <mstenta[m]> | Main differences from the previous approach: |
| [15:36:45] | <mstenta[m]> | - No longer using `#parents` |
| [15:36:45] | <mstenta[m]> | - Setting the string value in the `farm_map_input` form element's validation method |
| [15:36:45] | <mstenta[m]> | - Overriding `#element_validate` in `GeofieldWidget` to NOT use `farm_map_input`'s validation method 🤷 |
| [15:37:09] | <mstenta[m]> | The last point is the only bit I feel weird about... |
| [15:37:44] | <mstenta[m]> | But with that, and the only minor changes (in `FarmMapInput::valueCallback()` and `GeofieldWidget`), everything appears to be working |
| [15:37:54] | <mstenta[m]> | s/only/other/ |
| [15:40:18] | <mstenta[m]> | It does all make sense... not that "weird" I suppose... just a touch less elegant maybe. But that might just be the nature of these unique circumstances we're navigating. |
| [15:40:35] | <mstenta[m]> | Anyway - signing off. Take a look if you are able. Thanks again for helping with this! :-) |