GatherPress / gatherpress

Powering our community's event management needs.
https://wordpress.org/plugins/gatherpress/
GNU General Public License v2.0
84 stars 31 forks source link

Venue post_type does not take care about the WordPress Geodata Standard. #560

Open mauteri opened 7 months ago

mauteri commented 7 months ago

https://codex.wordpress.org/Geodata

carstingaxion commented 6 months ago

Thanks for keeping this @mauteri.

I wanted to give some more information on my initial attempt and why the WordPress Geodata Standard could be from interest for GatherPress' gp_venue post type and maybe even for the gp_event post type as well.

The codex says:

WordPress does not include any geographic functionality or functions to deal with Geodata itself [...]. But there is a standard for how to store geodata, so that plugins can interoperate.

The W3C Geolocation API([1]), introduced in 2014, offers the additional geolocation properties of altitude, accuracy, altitudeAccuracy, speed, and heading, which if adopted by any plugin to ensure compatibility with this, should be stored with the geo_ prefix.

If GatherPress wanted to follow this standard, it must make sure to save geo-related data within specially named custom-fields aka post_meta following this given format:

Because of the required fields for latitude and longitude, it's not possible to provide the needed data with the current version of GatherPress, the required data is missing.

So we would at least need to:

While working on #638 I found out and tested for the first time, if and that it's possible to work with non-existent post_meta. This allows GatherPress to keep its current data structure, while still aligning with the WordPress Geodata Standard.

Example pseudo-post_meta for geo_address

  1. Register the post_meta (like normal)

    'geo_address' => array(
    'auth_callback'     => function () {
        return current_user_can( 'edit_posts' );
    },
    'sanitize_callback' => 'sanitize_text_field',
    'show_in_rest'      => true,
    'single'            => true,
    'type'              => 'string',
    ),
  2. Load a filter to hook into the post_meta retrival

    \add_filter(
    'get_post_metadata',
    array( $this, 'get_pseudo_post_meta' ),
    10,
    3
    );
  3. Make sure data is delivered

    function get_pseudo_post_meta( $metadata, $object_id, $meta_key ): mixed {
    if ( ! in_array( $meta_key, ['geo_latitude','geo_longitude','geo_address'] ) ) {
        return $metadata;
    }
    $venue_information = (array) json_decode( get_post_meta( $object_id, 'venue_information', true ) );
    $output = [];
    
    switch ($meta_key) {
        case 'geo_latitude':
            $output[] = '48.856613'; // Paris
            break;
    
        case 'geo_longitude':
            $output[] = '2.352222'; // Paris
            break;
    
        case 'geo_address':
            $output[] = ( isset( $venue_information['fullAddress'] ) ) ? $venue_information['fullAddress'] : '';
            break;
    
        default:
            break;
    }
    
    return $output;
    }

This worked very well and I was able to see my pseudo-post_meta in action, using the Simple Location – WordPress plugin, which is following the WordPress Geodata Standard.

So, why not start saving latitude and longitude and aligning with the WordPress Geodata Standard?!!

shawfactor commented 6 months ago

I don’t think filtering the metadata is the right approach at all. Surely the plugin should just save the metadata using the right fields as described by the standard

More broadly saving all the venue information in one field in post meta is far from ideal anyway. In an ideal world every separate bit of information would have its own meta_key.

carstingaxion commented 6 months ago

Thank you for sharing your opinion @shawfactor

Surely the plugin should just save the metadata using the right fields as described by the standard

Yes.

More broadly saving all the venue information in one field in post meta is far from ideal anyway.

Yes. Yes.

In an ideal world every separate bit of information would have its own meta_key.

Yes. Yes. Yes.

I don’t think filtering the metadata is the right approach at all.

Yes. Yes. Yes. Yes.

But...

I was a little bit afraid of touching the principal data-architecture, because I only know about GatherPress since a short time.

I haven't measured it, but thought about this in particular for a while now: I think there shouldn't be any negative performance-impacts by providing non-existent meta-data based on existing meta-data.

And because we are going to render this data using the Block Bindings API, we want to create a render callback either.

mauteri commented 3 months ago

@stephenerdelyi @jmarx this is the data ticket for venues

MervinHernandez commented 3 months ago

✅ Viewed

MervinHernandez commented 3 months ago

✅ Reviewed June 22 = Confirming this has been discussed to be merged into the OpenStreetMaps work.

mauteri commented 2 months ago

@MervinHernandez this ticket opened up a bit of a can of worms for me. The approach above was a bit buggy because of the nature of the block editor I believe. I've punted this ticket to 0.31.0 because I think the data just needs to be overhauled as discussed above. I think we might even want to break into multiple Venue blocks so Address, Phone Number, and Website aren't tied to tightly together, but I want to chat about that a bit on Friday.

carstingaxion commented 2 months ago

I think we might even want to break into multiple Venue blocks so Address, Phone Number, and Website aren't tied to tightly together,

Your statement surprises me a bit, because this separation of concerns is exactly what I described in #626 (in general), in #629 (especially for the new venue block), and what I'm aiming for.

Given that I have still not described the details of #638, I can understand your confusion.

Overall the refactored, venue-related blocks will be IMHO :

When using those blocks from within an event- or other post type, you'll have to wrap them in a new venue-details block (#629), which is already working and can be tested in the GatherPress block playground.

mauteri commented 2 months ago

I think we might even want to break into multiple Venue blocks so Address, Phone Number, and Website aren't tied to tightly together,

Your statement surprises me a bit, because this separation of concerns is exactly what I described in #626 (in general), in #629 (especially for the new venue block), and what I'm aiming for.

Given that I have still not described the details of #638, I can understand your confusion.

Overall the refactored, venue-related blocks will be IMHO :

When using those blocks from within an event- or other post type, you'll have to wrap them in a new venue-details block (#629), which is already working and can be tested in the GatherPress block playground.

Yes, this is where I ended up too, though I could see Map / Address as the same block and they are tightly coupled. Website and Phone can be their own blocks.

Don't be surprised by me :-) A bunch of these were written years ago and I'm starting to look at things through a fresh, new lens. How we saw the project 2+ years ago and how we see it now has changed, and so has a lot of the components of it.

carstingaxion commented 1 month ago

Let's make sure, that the new metas have revisions enabled. IMO this makes sense for all (the formerly mentioned) meta, which just needs to be registered separately.

https://make.wordpress.org/core/2023/10/24/framework-for-storing-revisions-of-post-meta-in-6-4/