berlindb / core

All of the required core code
MIT License
256 stars 29 forks source link

Allow Field Types/Format List to Be Supplied #137

Closed spencerfinnell closed 2 years ago

spencerfinnell commented 2 years ago

I'm not exactly sure what this would look like, but I thought I would log it in case someone else runs in to something similar.

Working with Stripe objects, I have a table with a column object_id. It turns out WordPress sets a default field type for object_id, and other column names. This list of field types are used to prepare the SQL statement and format values accordingly when a field list and corresponding format list are not supplied during $wpdb::insert or $wpdb::update operations.

BerlinDB does not pass a format list when calling $wpdb::insert( string $table, array $data, array|string $format = null ) causing the WordPress core list to be used. This was causing Stripe Object IDs, which are strings, to be converted to an integer.

My workaround was to name the column _object_id and shim the insert and update operations to add the _ prefix. Other workarounds might be more involved and therefore maybe unnecessary, but again, just logging this for reference. Perhaps a format list could be auto generated based on columns supplied to Schema?

Semi-related: #124

JJJ commented 2 years ago

My workaround was to name the column _object_id and shim the insert and update operations to add the _ prefix.

Ouch! That's really annoying to have to do.

Perhaps a format list could be auto generated based on columns supplied to Schema?

This is the way, IMO.

We should prioritize fixing this ASAP.

JJJ commented 2 years ago

The Query class needs a prettier (and more useful) version of:

$patterns = array_intersect_key(
    wp_list_pluck( $this->get_columns(), 'pattern', 'name' ),
    $save,
);

...where $save is from Query::insert_item() and update_item().

Would be nice to confirm whatever we use is capable of using the default Column values.

JJJ commented 2 years ago

Realizing now that pattern probably should have been format.

Maybe we can alias this at a later date. Noting here for the future.

JJJ commented 2 years ago

Note: fixing #124 with the same PR, imminently.

JJJ commented 2 years ago

Fixed for 2.1.0.