Add hook in wp_ajax_save_attachment for additional attachment fields

[griffinjt]

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.

[griffinjt]

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 successful update_post_meta/wp_update_post call.

[griffinjt]

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. ;-)

[bradyvercher]

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 );

[griffinjt]

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.

[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.

[griffinjt]

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. :-)

[griffinjt]

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.

[bradyvercher]

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?

[wonderboymusic]

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

[pputzer]

@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:(

[antpb]

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.

[sc0ttkclark]

Thank you @antpb for helping revive this, it's definitely needed and I'm eager to implement it in Pods myself.

[costdev]

Patch refreshed against trunk and a docblock added.

https://core.trac.wordpress.org/ticket/23148

[audrasjb]

Ah ah you beat me @costdev 😅

(by the way your docblock is better than mine)

[audrasjb]

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 :)

[costdev]

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)?

[audrasjb]

Yeah your patch looks great to me :)

Self-assigning for commit.

[audrasjb]

In 55106:

Media: Add an action hook on wp_ajax_save_attachment().

This changeset introduces the wp_ajax_save_attachment action hook, triggered after an attachment has been updated and before the JSON response is sent. For example, it allows developers to update any additional attachment fields that have been rendered by extending the media.view.Attachment.Details subview.

Props griffinjt, bradyvercher, pputzer, antpb, sc0ttkclark, audrasjb, costdev, hellofromTonya.
Fixes #23148.

Closed in favor of costdev's patch

[SergeyBiryukov]

In 55111:

Docs: Further clarify the wp_ajax_save_attachment filter parameters description.

Follow-up to [55106].

See #23148.

[azaozz]

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".

[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. 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.

[azaozz]

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.

[sc0ttkclark]

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.

[sc0ttkclark]

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.

[azaozz]

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 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

[costdev]

@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?

[audrasjb]

In 55450:

Media: Rename the wp_ajax_save_attachment action hook to wp_ajax_save_attachment_updated.

This changeset 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. This also add a dockblock note to indicate that when checking if an action is being done, doing_action( 'wp_ajax_save-attachment' ) may be used if that is more convenient.

Follow-up to [55106].

Props azaozz, sc0ttkclark, costdev.
Fixes #23148

committed in https://core.trac.wordpress.org/changeset/55450

[azaozz]

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 or pre_post_{$field}, and {$field}_pre.
  • wp_insert_post_empty_content,
  • wp_insert_attachment_data,
  • pre_post_update,
  • edit_attachment,
  • attachment_updated,

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.

[azaozz]

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:

1. An example or a link to some existing code that will be replaced when this action becomes available.
2. Brief explanation why it is not possible to use attachment_updated with doing_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.

[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 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.

[SergeyBiryukov]

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 of doing_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.

[costdev]

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.

[audrasjb]

In 55474:

Media: Revert wp_ajax_save_attachment_updated hook.

This changeset removes the wp_ajax_save_attachment_updated hook by reverting [55106], [55111], and [55450], to give it more time for further discussions as there are still concerns about whether this hook is necessary.

Follow-up to [55106], [55111], [55450].

Props costdev, SergeyBiryukov, peterwilsoncc, azaozz.
See #23148.

[audrasjb]

Reverted. Moving for 6.3 consideration.

[sc0ttkclark]

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.

[antpb]

Moving future release as we have not still agreed on if this is duplicating existing features. Once justification for this is established we can move back into a release milestone.