Make WordPress Core

Opened 11 years ago

Last modified 9 months ago

#23148 reopened enhancement

Add hook in wp_ajax_save_attachment for additional attachment fields

Reported by: griffinjt's profile griffinjt Owned by: audrasjb's profile audrasjb
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)

23148.diff (365 bytes) - added by griffinjt 11 years ago.
23148.2.diff (669 bytes) - added by costdev 14 months ago.
Patch refreshed against trunk and a docblock added.

Download all attachments as: .zip

Change History (67)

#1 @SergeyBiryukov
11 years ago

  • Type changed from defect (bug) to enhancement

@griffinjt
11 years ago

#2 @griffinjt
11 years ago

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.

#3 @SergeyBiryukov
11 years ago

  • Keywords has-patch added; needs-patch removed

#4 @griffinjt
11 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

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

Version 0, edited 11 years ago by griffinjt (next)

#5 @SergeyBiryukov
11 years ago

  • Milestone changed from Awaiting Review to 3.6

#6 @wpsmith
11 years ago

  • Cc t@… added

#7 @goto10
11 years ago

  • Cc dromsey@… added

#8 @mordauk
11 years ago

  • Cc pippin@… added

#9 @sc0ttkclark
11 years ago

  • Cc lol@… added

#10 @chrisvanpatten
11 years ago

  • Cc chris@… added

#11 @mordauk
11 years ago

  • Cc pippin@… removed

#12 follow-up: @bradyvercher
11 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 @griffinjt
11 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: @bradyvercher
11 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 @griffinjt
11 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 @griffinjt
11 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 @bradyvercher
11 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?

#18 @ryan
11 years ago

  • Milestone changed from 3.6 to Future Release

#19 @wonderboymusic
9 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 @pputzer
8 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:(

#21 @SergeyBiryukov
8 years ago

  • Milestone set to Awaiting Review

This ticket was mentioned in Slack in #core-media by pepe. View the logs.


7 years ago

This ticket was mentioned in Slack in #core-media by pepe. View the logs.


7 years ago

This ticket was mentioned in Slack in #core-media by joedolson. View the logs.


18 months ago

#25 follow-up: @antpb
18 months 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 @sc0ttkclark
18 months 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.


18 months ago

This ticket was mentioned in Slack in #core by costdev. View the logs.


14 months ago

@costdev
14 months ago

Patch refreshed against trunk and a docblock added.

#30 @audrasjb
14 months ago

Ah ah you beat me @costdev 😅

(by the way your docblock is better than mine)

Last edited 14 months ago by audrasjb (previous) (diff)

#31 @audrasjb
14 months 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 @costdev
14 months 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 @audrasjb
14 months ago

  • Owner set to audrasjb
  • Status changed from reopened to accepted

Yeah your patch looks great to me :)

Self-assigning for commit.

#34 @audrasjb
14 months ago

  • Keywords commit added

#35 @audrasjb
14 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

@audrasjb commented on PR #3874:


14 months ago
#36

Closed in favor of costdev's patch

#37 @SergeyBiryukov
14 months ago

In 55111:

Docs: Further clarify the wp_ajax_save_attachment filter parameters description.

Follow-up to [55106].

See #23148.

#38 in reply to: ↑ 25 @azaozz
13 months 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".

Last edited 13 months ago by azaozz (previous) (diff)

#39 follow-up: @sc0ttkclark
13 months 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: @azaozz
13 months 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.

Last edited 13 months ago by azaozz (previous) (diff)

#41 in reply to: ↑ 40 ; follow-up: @sc0ttkclark
13 months 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: @sc0ttkclark
13 months 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 @azaozz
13 months 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.

Last edited 13 months ago by azaozz (previous) (diff)

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


13 months ago

This ticket was mentioned in PR #4151 on WordPress/wordpress-develop by @costdev.


13 months 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: @costdev
13 months 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.


13 months ago

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


13 months ago

#49 @audrasjb
13 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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

#51 @adamsilverstein
13 months ago

  • Keywords needs-dev-note added

#52 in reply to: ↑ 46 @azaozz
13 months 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 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.

Last edited 13 months ago by azaozz (previous) (diff)

#53 in reply to: ↑ 42 @azaozz
13 months 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:

  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.

#54 follow-up: @peterwilsoncc
13 months 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 @SergeyBiryukov
13 months 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 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.

Last edited 13 months ago by SergeyBiryukov (previous) (diff)

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


13 months ago

#57 @costdev
13 months 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.

#58 @audrasjb
13 months ago

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.

#59 @audrasjb
13 months ago

  • Milestone changed from 6.2 to 6.3

Reverted. Moving for 6.3 consideration.

#60 @sc0ttkclark
13 months 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.

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


13 months ago

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


11 months ago

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


10 months ago

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


9 months ago

#65 @antpb
9 months ago

  • Milestone changed from 6.3 to Future Release

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.

Note: See TracTickets for help on using tickets.