Opened 12 years ago
Last modified 20 months ago
#23148 reopened enhancement
Add hook in wp_ajax_save_attachment for additional attachment fields
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | 3.5 |
Component: | Media | Keywords: | has-patch 2nd-opinion dev-feedback needs-dev-note needs-docs |
Focuses: | Cc: |
Description
As of now, wp_ajax_save_attachment
only looks for the attachment title, caption, description and alt text fields. Without a hook, there is no way to extend the new media system properly and take advantage of the save functionality.
There should be a hook provided at the end of this function (before the JSON response is sent back) that allows devs to update any additional attachment fields that have been rendered by extending the media.view.Attachment.Details
subview.
Attachments (2)
Change History (67)
#4
@
12 years ago
As an update, I have tested out this patch and it works as expected. Here is my code as a proof of concept for both rendering the custom field template and saving the data: https://gist.github.com/f3d93e78ed118b283938. Note that the call to "wp_ajax_save_attachment" is the hook I have added in the patch.
It would be great if the patch could be committed to 3.5.1 so that I can actively start using the new media manager in both my plugins and client work. ;-)
#12
follow-up:
↓ 13
@
12 years ago
- Cc brady@… added
Any fields added using the backward compatible filter attachment_fields_to_edit
are already rendered in the new media modal and can be saved by hooking into attachment_field_to_save
, which is fired by wp_ajax_save_attachment_compat()
. These fields can also be registered to work on the new "Edit Media" screens, so I'm not sure a hook is really necessary here.
Here's how something similar would be done:
function attachment_url_field_23148( $form_fields, $post ) { $form_fields['attachment_url_23148'] = array( 'label' => 'Link', 'input' => 'text', 'value' => get_post_meta( $post->ID, 'attachment_url_23148', true ), 'show_in_edit' => false, ); return $form_fields; } add_filter( 'attachment_fields_to_edit', 'attachment_url_field_23148', 10, 2 ); function save_attachment_url_23148( $post, $data ) { if ( isset( $data['attachment_url_23148'] ) ) { update_post_meta( $post['ID'], 'attachment_url_23148', esc_url_raw( $data['attachment_url_23148'] ) ); } return $post; } add_filter( 'attachment_fields_to_save', 'save_attachment_url_23148', 10, 2 );
#13
in reply to:
↑ 12
@
12 years ago
Replying to bradyvercher:
Any fields added using the backward compatible filter
attachment_fields_to_edit
... These fields can also be registered to work on the new "Edit Media" screens, so I'm not sure a hook is really necessary here.
Red flags should be thrown up when you are required to use a backwards compatible filter with a new API. I disagree and believe this hook is necessary to promote good and future-minded code with the new media APIs. Beyond this, the new API is JavaScript, and I don't see promoting an entirely PHP solution as a good thing moving forward. This hook simply opens up the "model" portion of Backbone so that it can be utilized by other people properly extending Backbone's other APIs.
#14
follow-up:
↓ 15
@
12 years ago
No need for red flags; those filters are the API and have been since before the new media modal was implemented, so why not use them? Just trying to help.
#15
in reply to:
↑ 14
@
12 years ago
Replying to bradyvercher:
No need for red flags; those filters are the API and have been since before the new media modal was implemented, so why not use them? Just trying to help.
If I came off accusatory, I apologize - I promise I didn't mean it! Maybe I should have just said that it seems odd to use code that is now not the primary way to interact with the new API. Either way, I still think a hook here would be beneficial because it would allow you to take advantage of the new JS media code without having to write completely new JS glue to save your custom fields added with JS. :-)
#16
@
12 years ago
One other thing to note about the old code is that the new JS media manager does not recognize anything set by WP_Screen (at least from my testing), so unless there is another way to test, there isn't a way to limit where the attachment fields appear.
#17
@
12 years ago
No worries and good point on context-sensitive fields. In that case, data in the $_REQUEST
global is available to every action fired during the request, so edit_attachment
should eventually get called and work fine for your purposes:
function soliloquy_save_attachment_fields( $attachment_id ) { if ( defined( 'DOING_AJAX' ) && DOING_AJAX && ! empty( $_REQUEST['changes']['soliloquy_link'] ) ) update_post_meta( $attachment_id, '_soliloquy_image_link', esc_url_raw( $_REQUEST['changes']['soliloquy_link'] ) ); } add_action( 'edit_attachment', 'soliloquy_save_attachment_fields' );
Would something like that work?
#19
@
10 years ago
- Milestone Future Release deleted
- Resolution set to wontfix
- Status changed from new to closed
No one has chimed in to re-ask for this filter in 15 months, I think there are sufficient ways to accomplish what you want
#20
@
9 years ago
- Resolution wontfix deleted
- Status changed from closed to reopened
@wonderboymusic That's probably because there still are no tutorials on how to use the new Backbone API in plugins. The compat fields generated by attachment_fields_to_edit
are huge mess markup-wise and behave completely differently because they get thrown into a <table>
. I'm in the process of converting my plugin (Media Credit) to the new API and while it's possible to override yet another method on the JS side, it would be far nicer to be able to hook in to the existing wp_ajax_save_attachment
action.
On a not-so-thorough first reading of the code, I thought I could use the existing wp_update_attachment_metadata
filter for this - until I noticed it is only called for attachments of type audio
:(
This ticket was mentioned in Slack in #core-media by pepe. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-media by pepe. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-media by joedolson. View the logs.
2 years ago
#25
follow-up:
↓ 38
@
2 years ago
- Milestone changed from Awaiting Review to 6.2
There doesn't seem to be any reason _not_ to add this. Time has buried this ticket. Moving to 6.2 to get it visibility and a conclusion.
#26
@
2 years ago
Thank you @antpb for helping revive this, it's definitely needed and I'm eager to implement it in Pods myself.
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
2 years ago
This ticket was mentioned in Slack in #core by costdev. View the logs.
2 years ago
This ticket was mentioned in PR #3874 on WordPress/wordpress-develop by @audrasjb.
2 years ago
#29
#31
@
2 years ago
- Keywords needs-dev-note added
But I found something interesting to add, so I'm at least feeling a bit useful 🤪
This change needs to be mentioned in the Misc Changes devnote :)
#32
@
2 years ago
Sorry @audrasjb! 😂
Yeah I had to double-check the $post
, as it's ARRAY_A
rather than WP_Post
.
Additional props to @hellofromTonya from rubber-ducking with me on the appropriate hook position.
What do you think @audrasjb? Is 23148.2.diff ready for commit consideration, or is there anything else you think it might need (in terms of code/docs)?
#33
@
2 years ago
- Owner set to audrasjb
- Status changed from reopened to accepted
Yeah your patch looks great to me :)
Self-assigning for commit
.
@audrasjb commented on PR #3874:
2 years ago
#36
Closed in favor of costdev's patch
#38
in reply to:
↑ 25
@
2 years ago
- Keywords 2nd-opinion added; needs-dev-note commit removed
- Resolution fixed deleted
- Status changed from closed to reopened
Replying to antpb:
There doesn't seem to be any reason _not_ to add this.
I actually think there are at least couple of reasons :)
- This duplicates all the
save_post
hooks. - The ticket has had no traction for 10 years. This means nobody has any interest in using this action probably because there are several other hooks that can be used and most people are familiar with.
Imho this should be removed as the new action seems useless, and the ticket closed with "wontfix".
#39
follow-up:
↓ 40
@
2 years ago
@azaozz Without this action, we have no way to know that an attachment was added through the wp_ajax_save_attachment()
call except to sniff what we can from request vars. I continue to believe this action would offer an official way to hook into that flow.
I know you know that no traction on Trac tickets does not always correlate to the usefulness of the ticket. In this case, I believe that is because this is in an area that's less often dealt with: Attachment fields. The cool story here is that there was a hook for this attachment_fields_to_save
that helped but that's for the legacy media handling. The action implemented here allows for that and many other use-cases stemming from the AJAX save_attachment action.
#40
in reply to:
↑ 39
;
follow-up:
↓ 41
@
2 years ago
Replying to sc0ttkclark:
@azaozz Without this action, we have no way to know that an attachment was added through the
wp_ajax_save_attachment()
call except to sniff what we can from request vars.
That's not entirely true. There's the DOING_AJAX
and even wp_doing_ajax()
. Also all WP AJAX calls are done from hooks, see https://core.trac.wordpress.org/browser/tags/6.1/src/wp-admin/admin-ajax.php#L104 and https://core.trac.wordpress.org/browser/tags/6.1/src/wp-admin/admin-ajax.php#L164.
It seems it is quite easy to determine when wp_ajax_save_attachment()
was used without having a new action. My favorite would be: if ( doing_action( 'wp_ajax_save-attachment' ) ) ...
I continue to believe this action would offer an official way to hook into that flow.
Yes, it will. The question is: who needs that action?
I know you know that no traction on Trac tickets does not always correlate to the usefulness of the ticket.
Correct. On the other hand "no traction for 10 years" usually means that there are other ways to achieve the same result, or that the result is not needed any more.
In this case, I believe that is because this is in an area that's less often dealt with: Attachment fields. The cool story here is that there was a hook for this
attachment_fields_to_save
that helped but that's for the legacy media handling. The action implemented here allows for that and many other use-cases stemming from the AJAX save_attachment action.
My biggest problems here are these two:
- For a very very long time this was not needed. It doesn't seem needed now. Imho this looks like a ticket that was opened because of plugin development and the author wanted an "easy way" to do something specific. Then the author found an existing/different/better way to achieve the same result and the ticket became stale.
- When adding a new hook WordPress takes on the responsibility to maintain it "forever". As this hook is not used in core, and there doesn't seem to be any pressing need for it right now, and there are no use cases or even any evidence that it may be useful (other than it may make it a little easier for few plugins in some cases), I don't think it is a good idea to add more "backwards compatibility requirements" to the core code.
If you believe this will be used by 100-200 plugins, and it will make performing certain task so much easier, by all means lets add it. Would be great to have at least some indication of that. But if something is not used in core, and seems nobody needed it for 10 years, and WP will have to maintain it forever (even if only 2-3 plugins will ever use it), I'm not sure it is a good idea to add it.
#41
in reply to:
↑ 40
;
follow-up:
↓ 43
@
2 years ago
Replying to azaozz:
But if something is not used in core, and seems nobody needed it for 10 years, and WP will have to maintain it forever (even if only 2-3 plugins will ever use it), I'm not sure it is a good idea to add it.
I'll concede there that since before this ticket was created -- all plugin developers looking to integrate in this area have been using workarounds to make it all work. So now we're looking at 10+ years of code in these plugins and it would be slow going for those existing plugins to switch to the better hook.
I don't know how many plugin authors will recognize this new hook is a better replacement for what they had used and that ultimately will lead to a slow adoption. They most likely already forgot about why they used their workaround (unless they left code comments) and will leave that code in place. They won't change it because their old solution works just fine for them.
But the new developers / projects, they have a chance at having a better life than us :)
I still think it would be the right way to handle this sort of thing. Otherwise, doing a doing_action()
would be the right way to handle most secondary and tertiary customizations without any other context available.
#42
follow-up:
↓ 53
@
2 years ago
I'll be implementing this in Pods Framework if/when the hook becomes available in WP core, so there's a drop in the bucket but at least it's a start. I know I could convince the WPEngine team building ACF to also join in usage.
#43
in reply to:
↑ 41
@
2 years ago
- Keywords changes-requested added
Replying to sc0ttkclark:
developers looking to integrate in this area have been using workarounds to make it all work
I wouldn't call using the wp_ajax_save-attachment
action a workaround. It's rather the "right way to hook into any AJAX call".
They won't change it because their old solution works just fine for them.
Yep, exactly.
But the new developers / projects, they have a chance at having a better life than us :)
The question is about the balance: the maintenance "price" the WP project will have to pay vs. the minor convenience for few developers :)
Actually thinking more about it the new wp_ajax_save_attachment
action is almost a duplicate of the existing wp_ajax_save-attachment
action, both in functionality and as "action name". This would probably be confusing.
Still don't see a point in replacing doing_action( 'wp_ajax_save-attachment' )
with add_action( 'wp_ajax_save_attachment', ... )
, but if everybody thinks it is a good idea, lets keep it. However thinking the new action at least should have a better/different name to avoid possibility of confusion.
Also thinking that if the new action is renamed and remains in core, the docblock should mention that it is very similar to the wp_ajax_save-attachment
action, and that it can be replaced with doing_action( 'wp_ajax_save-attachment' )
if that is more convenient.
This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.
2 years ago
This ticket was mentioned in PR #4151 on WordPress/wordpress-develop by @costdev.
2 years ago
#45
This renames the wp_ajax_save_attachment
action to wp_ajax_save_attachment_updated
to avoid confusion with the similarly named wp_ajax_save-attachment
action.
When checking if an action is being done, doing_action( 'wp_ajax_save-attachment' )
may be used if that is more convenient.
Trac ticket: https://core.trac.wordpress.org/ticket/23148
#46
follow-up:
↓ 52
@
2 years ago
- Keywords dev-feedback added; changes-requested removed
@azaozz I have submitted PR 4151 which renames the wp_ajax_save_attachment
action to wp_ajax_save_attachment_updated
which is more in line with when the action is specifically fired.
I've also added a note in the docblock that when checking if an action is being done, doing_action( 'wp_ajax_save-attachment' )
may be used if that is more convenient.
How do these changes sound to you?
This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.
2 years ago
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
2 years ago
@audrasjb commented on PR #4151:
2 years ago
#50
committed in https://core.trac.wordpress.org/changeset/55450
#52
in reply to:
↑ 46
@
2 years ago
Replying to costdev:
I've also added a note in the docblock that when checking if an action is being done,
Sounds good. I'd like to expand the docs a bit more and explain that:
- There is a
wp_ajax_save-attachment
action that is very similar and is fired earlier. - The new
wp_ajax_save_attachment_updated
is full equivalent (duplicate?) of using any of the "save_post" hooks:- The
sanitize_post_field()
filters:pre_{$field}
and{$field_no_prefix}_save_pre
orpre_post_{$field}
, and{$field}_pre
. wp_insert_post_empty_content
,wp_insert_attachment_data
,pre_post_update
,edit_attachment
,attachment_updated
,
- The
and checking where the data comes from by using if ( doing_action( 'wp_ajax_save-attachment' ) )
.
As that will be just a docs update it can be added at any time. Also seems this should be mentioned in the dev note.
#53
in reply to:
↑ 42
@
2 years ago
- Keywords needs-docs added
Replying to sc0ttkclark:
I'll be implementing this in Pods Framework
Sorry but I'm still struggling to understand why the attachment_updated
hook cannot be used in this case. Would be really helpful if you could provide some more info:
- An example or a link to some existing code that will be replaced when this action becomes available.
- Brief explanation why it is not possible to use
attachment_updated
withdoing_action( 'wp_ajax_save-attachment' )
.
As far as I see wp_ajax_save_attachment_updated
is a duplicate of attachment_updated
.
Guessing that the reported was not aware of the wp_ajax_save-attachment
action and the possibility to use doing_action( 'wp_ajax_save-attachment' )
. However it seems that when attempting to modify the saving of an attachment, one of the other hooks (mentioned in the previous comment) will have to be used anyway. This seems limit even further the usefulness of the new wp_ajax_save_attachment_updated
.
Also, are you going to be writing the dev. note? Could you also include the above list of other hooks that are triggered. I'll make sure add them to the docblock and link to this ticket for further reference.
#54
follow-up:
↓ 55
@
2 years ago
I could be convinced either way but I kind of agree with Aaron here, that it doubles up a lot of actions from the wp_insert_post()
function. Documenting the use of doing_action( 'wp_ajax_save-attachment' )
adds to the confusion, as that seems something that can be done in the existing hooks.
It would be good to know what the advantage is that this brings that could not be achieved by:
<?php add_action( 'wp_after_insert_post', function( $post, $is_update, $post_before ) { if ( ! doing_action( 'wp_ajax_save-attachment' ) ) { return; } // Etc, etc. }, 10000, 3 );
wp_after_insert_post
chosen because it's the hook that runs immediately prior to this one but this could apply to many of the hooks mentioned earlier.
It's also worth noting that a few plugins replace the wp_ajax_save-attachment
and die prior to the core version running (the highest I saw has 200K installs) so advising people move to the new hook may cause problems until such plugins implement it. I've done it once or twice in my client services days when implementing DAMs.
#55
in reply to:
↑ 54
@
2 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Replying to peterwilsoncc:
I could be convinced either way but I kind of agree with Aaron here, that it doubles up a lot of actions from the
wp_insert_post()
function. Documenting the use ofdoing_action( 'wp_ajax_save-attachment' )
adds to the confusion, as that seems something that can be done in the existing hooks.
Indeed, this looks a lot like #28112 to me, where the bulk_edit_posts
hook was reverted in [55265] pending confirmation that it is actually needed, especially after the ticket reporter has apparently been able to achieve the goal using the existing hooks.
Reopening to either further improve the docs as suggested in comment:52, or revert for now and move to 6.3 to get a better idea of whether this hook is needed, and how it should be implemented.
This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.
2 years ago
#57
@
2 years ago
This ticket was discussed during the bug scrub.
As we're approaching RC1 and there are still concerns about whether this hook is necessary, myself and @mukesh27 agree that this should be reverted and moved to 6.3 for further discussion.
#60
@
2 years ago
I'll do a deep dive on this from how I'm using hooks around this area right now to provide better feedback at some point in the next few weeks.
I've added a patch that adds a hook right after WordPress internally updates the post. Passing two params,
$post
and$changes
, is sufficient for a user to have the information needed to make a successfulupdate_post_meta/wp_update_post
call.