Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.I think an important field is missing: GEO with a syntax like:
GEO:+47.976177;+7.81807
(Remember that "LOCATION" carries the adress.)
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | date_ical-add-geo-support-2203085.patch | 7.46 KB | coredumperror |
Comments
Comment #1
geek-merlinNote: it should be enough to extract the combined lat/lon pair and pass the job of parsing this to geofield.
Comment #2
coredumperror CreditAttribution: coredumperror commentedHuh, I wasn't aware of the GEO field in iCal until you pointed it out. It shouldn't be too hard to implement, though I'm too busy to get to work on it immediately.
Comment #3
geek-merlinIt should be as easy as this.
Tested taht this gives us a new mapping source.
Did not test yet that the result of an import is suitable for geofield mapper.
Comment #4
geek-merlinIt's not that easy.
The library itself does a lot of magick and returns an array keyed by "latitude"/"longitude".
So we have to postprocess that - so i had to do a simple extension of the internal api.
(We might use this for now and refactor anytime later)
Furthermore i noticed that geofield mapper has some quirks which i noted in the source descriptions (which maybe saves us some support issues).
Improved patch flying in. Tested on some feeds and used in production to map to a geofield.
Comment #5
geek-merlinComment #6
coredumperror CreditAttribution: coredumperror commentedIs there any reason to support importing the Latitude and Longitude values separately? Offering additional sources, especially ones that don't work with the Geofield mapper, seems more likely to confuse users than to assist them.
Also, I noticed something interesting: Geofield itself offers a Feeds Source, called "Geofield (combined)". It might be possible to write our parser in such a way that it's output is supported by Geofield's
geofield_feeds_combined_source()function. I'm going to look into that.Comment #7
geek-merlinhey robert,
* im am using this in production with the combined geofield mapper (that's why i wrapped the array the way i did)
* why parse lat/lon separately? because there may be other mappers. of course we really might move the "not working with geofield" warning from the description (which is not shown in feeds) to the title. but no problem for me if we remove this altogether and wait until someone requests it.
Comment #8
coredumperror CreditAttribution: coredumperror commentedI was in the middle of a major tweak to your code when I got pulled off it for a high-priority project at work. I may be able to get back to it later today or tomorrow.
Comment #9
geek-merlinhumptydumpty...
Comment #10
coredumperror CreditAttribution: coredumperror commentedJust as soon as that high-priority project finished, I got another one shoved onto me unexpectedly. I've been wanting to get back to both Date iCal and s3fs for months, and just haven't been able to do so. :( At this time, I just don't know when I'll have time for my contrib modules again.
Comment #11
coredumperror CreditAttribution: coredumperror commentedHere's my take on a patch for GEO import support (finally!). I looked at how other importers work, and discovered the existence of
hook_feeds_parser_sources_alter(). The Geofield module uses that hook to define the "Geofield (combined)" feeds mapping source.So I set up Date iCal to properly retrieve the full list of available mapping sources, and to handle the source that the Geofield module defines. If the user sets up a mapping for that source, Date iCal will parse the GEO field from the imported event, and then format the data in a way that Geofield's
geofield_feeds_combined_source()function understands.Since this is a significant change to the import system, I don't want to push this to the dev version until some more people have had a chance to test it out. I've tested it myself quite a bit, but other Drupal setups might pose problems. So I've only attached the patch here, rather than pushing it to git. I would appreciate whatever feedback you guys can offer!
Comment #12
geek-merlinCOOL, code looks great.
I have a way too high workload too these days, but as soon as i can i will test this.
BTW: we have tests, so i think somewhere in the project settings you might switch on the testbot for patches.
Comment #13
coredumperror CreditAttribution: coredumperror commentedWell, we have a "tests" folder, but it's not actually testbot-compatible code. It's just a bunch of ical feeds which exercise all the code paths in the parser code.
Comment #15
coredumperror CreditAttribution: coredumperror commentedWhen I created the workaround for buggy UNTIL values for another issue, I forgot to take into account that the work I did was working from the version of Date iCal with this patch applied. I figured it wouldn't be the end of the world to push this code to git, though, since it seems to be working just fine. IF you do find any problems, though, please let me know!
Comment #16
coredumperror CreditAttribution: coredumperror commented