Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Store 'file' ID only, not URL #1488

Open
robertandrews opened this issue Apr 1, 2023 · 9 comments
Open

Store 'file' ID only, not URL #1488

robertandrews opened this issue Apr 1, 2023 · 9 comments

Comments

@robertandrews
Copy link

robertandrews commented Apr 1, 2023

Is your feature request related to a problem? Please describe.
No specific problem, but I don't understand why it's necessary to store the file URL when local attachments are the focus.
It's not a guarantee that the size variant given by the URL is the right one to serve.
The file URL is always resolvable by looking up the attachment by ID.
This allows for serving of the appropriate size etc.

Describe the solution you'd like
Docs say: "This field type will also store the attachment ID (useful for getting different image sizes."
But suppress storage of the file URL / explain why storing this is beneficial.

Describe alternatives you've considered
Any hook/action that would allow suppression of the URL save.
The 'url' => false option seems to pertain only to the frontend (admin) visibility of the URL, rather than actually making URL storage false.

Additional context
I'm aiming to move from ACF to CMB2 because it stored tens of thousands of needless rows (a meta item's pointer to the field descriptor). I'm trying to minimise database overhead.

@tw2113
Copy link
Contributor

tw2113 commented Apr 1, 2023

One quick off the top of my head benefit is not having to look up and fetch a URL, it's just already stored there.

I think this topic has come up in the past and there was decision based on backwards compatibility potential issues, but it's been awhile and my memory may not be complete.

@robertandrews
Copy link
Author

Thanks for the reply, Michael.
Maybe there's a case for saying: "That's what wp_postmeta is for" when it comes to saving an attachment URL?
I couldn't find a way to intervene and suppress CMB2 from saving the URL.
Great plugin; hoping to move from ACF 👍.

@chrisgherbert
Copy link

Another downside to saving the URL is that I've many times ended up forgetting to actually use the attachment rather than the direct value of the meta. Then once I'm using the code in a different environment (say, moving from development or staging to the production version), the URL doesn't work.

Not a huge deal, but removing the URL would prevent that kind of mistake from happening in the first place.

@robertandrews
Copy link
Author

robertandrews commented Apr 2, 2023

Can cmb2_override_logo_meta_value and cmb2_override_logo_meta_save (or other filters) be used somehow to enable getting/setting only of a media ID when saving a file on a term?

(Both the field name and meta_key are logo). Ideally, that would be minus the _id suffix.

I see the linked examples (eg https://github.com/CMB2/CMB2-Snippet-Library/blob/master/filters-and-actions/override-cmb2-data-source.php#L73-L98), though I'm not sure if that's what it's for, or how it would be done...

eg. If you had a function hooked to cmb2_override_logo_meta_save, could it then update_term_meta($term_id, 'logo', 'but_wheres_the_file_id') specifically? Is that what this is designed for?

@robertandrews
Copy link
Author

I tried this pair. Any comments, please?

Saving

add_filter('cmb2_override_logo_meta_save', 'override_meta_save', 10, 4);
    function override_meta_save($override, $args, $field_args, $field)
    {
        $term_id = $args['id'];
        $logo_id = $field->data_to_save['logo_id'];

        $updated = update_term_meta($term_id, $args['field_id'], $logo_id);

        return !!$updated;
    }

... which successfully saves a new value as meta_key logo (same as field id) - but does not succeed in suppressing the save for the default logo_id formulation. How can I do this?

Retrieving

add_filter('cmb2_override_logo_meta_value', 'override_meta_value', 10, 4);
    function override_meta_value($data, $object_id, $args, $field)
    {
        // Get term meta field 'logo
        $logo_id = get_term_meta($object_id, $args['field_id'], true);

        // Get attachment thumbnail object
        $logo = wp_get_attachment_image_src($logo_id, 'thumbnail');

        // Return URL of image to show
        return $logo[0];

    }

On the term edit page, this successfully fetches and shows the 150x150 thumbnail of the ID which is set in logo - not logo_id and not the full size image.
It's nice. I haven't tested it on anything in the front end yet.

@tw2113
Copy link
Contributor

tw2113 commented Apr 3, 2023

If I recall those override filters, returning non null values should assert itself. If you still returned null to the filter, it'd carry on and do its thing. Anything except null would use and return that value instead.

@robertandrews
Copy link
Author

If I recall those override filters, returning non null values should assert itself. If you still returned null to the filter, it'd carry on and do its thing. Anything except null would use and return that value instead.

Saving

Yeah... I can see how using the override succeeds in stopping the URL field getting set... though I can't quite figure out how to set the now-singular value as "logo" instead of "logo_id"

I might have to just convert all my existing "logo" meta_keys to "logo_id", to match CMB2's preference.

Retrieving

On the display side, I think a null value (result of wp_get_attachment_image_src()) results in PHP Notice: Trying to access array offset on value of type bool. So I have made return $logo[0]; conditional on it existing.

@robertandrews
Copy link
Author

The code workarounds I use above to save only an attachment ID to the meta, not an array also comprising the URL, hits a bump when it comes to file_list (multiple files).

I'm inheriting event_photos meta whose meta_value are like a:1:{i:0;s:6:"147242";} (singular) and a:3:{i:0;s:6:"146789";i:1;s:6:"146788";i:2;s:6:"146787";} (multiple).

As-is, the file_list in the admin will only show the ID, not the image.

That's different from the filter workaround I hit on for a single file, which just saves the ID and, on the display side, uses the ID to show the attachment thumbnail URL.

I think it's going to require writing a similar pair of filter functions, though I'm not sure what.
Do you have any pointers, please?

@tw2113
Copy link
Contributor

tw2113 commented Apr 7, 2023

Nothing from me, I haven't attempted to think your usecase through enough to have an approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants