Make WordPress Core

Opened 14 months ago

Closed 3 months ago

Last modified 2 months ago

#57897 closed enhancement (fixed)

REST attachments controller does not handle terms on creation

Reported by: swissspidy's profile swissspidy Owned by: swissspidy's profile swissspidy
Milestone: 6.5 Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-patch has-unit-tests has-testing-info add-to-field-guide
Focuses: rest-api Cc:

Description

\WP_REST_Attachments_Controller::create_item() does not call parent::create_item(), thus it also doesn't call \WP_REST_Posts_Controller::handle_terms(). That means it's impossible to assign terms to the freshly uploaded attachment.

rest_after_insert_attachment can be used to work around this, but I think it would make sense to have this built-in.

Requires only a few lines of code:

$terms_update = $this->handle_terms( $post_id, $request );

if ( is_wp_error( $terms_update ) ) {
        return $terms_update;
}

Change History (24)

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


14 months ago
#1

  • Keywords has-patch added; needs-patch removed

This PR fixes an issue where terms could not be assigned to newly uploaded attachments through the REST API, due to the \WP_REST_Attachments_Controller::create_item() function not calling parent::create_item() and thus not calling \WP_REST_Posts_Controller::handle_terms(). This PR adds the necessary lines of code to call handle_terms() in create_item(), allowing terms to be assigned to attachments through the REST API. The changes have been tested and work as expected.

Trac ticket: https://core.trac.wordpress.org/ticket/57897

#2 @swissspidy
11 months ago

  • Keywords needs-testing needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release

@TimothyBlynJacobs commented on PR #4209:


11 months ago
#3

Thanks for the patch @tanjimTC! This change looks good, could you add unit tests demonstrating that we can create an attachment with terms?

@spacedmonkey commented on PR #4209:


11 months ago
#4

Let's add a unit test.

#5 @swissspidy
9 months ago

  • Milestone changed from Future Release to 6.4

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


8 months ago

#7 @oglekler
8 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

This ticket has unit tests and is ready for testing.

This ticket was mentioned in Slack in #core-test by oglekler. View the logs.


8 months ago

#9 @Ankit K Gupta
7 months ago

  • Keywords needs-testing-info added

Hi @talldanwp Thanks for the PR, could you please add manual testing instructions to verify this fix?

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


7 months ago

#11 @oglekler
7 months ago

  • Keywords needs-unit-tests added; needs-testing has-unit-tests removed

This ticket was discussed during a bug scrub, Unit-tests are still missing, and we have 12 days left until Beta 1 when the opportunity window will be closed. Due to lack of activity, this is a candidate for Future release.

Add props to @mukesh27

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


7 months ago
#12

  • Keywords has-unit-tests added; needs-unit-tests removed

PR adds handle terms in the REST attachments controller to assign terms to the freshly created attachment.

Currently, \WP_REST_Attachments_Controller::create_item() does not call parent::create_item(), thus it also doesn't call \WP_REST_Posts_Controller::handle_terms(). That means it's impossible to assign terms to the freshly uploaded attachment. As suggested in the ticket description, PR adds handle_terms() in WP_REST_Attachments_Controller to fix this issue.

Trac ticket: https://core.trac.wordpress.org/ticket/57897

#13 @Dharm1025
7 months ago

  • Keywords has-testing-info needs-testing added; needs-testing-info removed

Testing Instructions

Steps to test

  1. Apply PR 5238
  2. Register a new taxonomy for the attachment object type with show_in_rest set to true OR add an already registered taxonomy to an attachment. For example:
    <?php
    register_taxonomy_for_object_type( 'category', 'attachment' );
    
  1. Send a REST API request to create an attachment with taxonomy terms. (For more information, refer to the WordPress REST API documentation).

Expected Results

  • ✅ Taxonomy terms included in the API request should be assigned to the newly created attachment.

cc: @ankit-k-gupta (as you requested testing instructions earlier)

@mukesh27 commented on PR #5238:


7 months ago
#14

Thank you, @iamdharmesh, for the unit tests. They appear to be solid to me. Let's wait for feedback from some of the maintainers before marking it for commit.

#15 @Ankit K Gupta
7 months ago

Test / Verification Report. ✅

Patch tested: https://github.com/WordPress/wordpress-develop/pull/5238

Env

  • WordPress - 6.4-alpha-20230918.155338
  • Web Server: Nginx
  • Chrome Version - 116
  • OS - macOS
  • Theme: Storefront
  • PHP - 7.4.0
  • Active Plugin - JSON Basic Authentication

Actual Results

  • ✅ Issue resolved with PR.


Test result

This pull request addresses an issue where terms could not be assigned to newly uploaded attachments through the REST API. Tested by following the above instructions.
Without/before the PR, taxonomies were not getting assigned when uploading media via REST API. After the PR, categories are assigned to the newly added attachment.

Video Demostration

https://www.loom.com/share/a933fead39624c3186143e1b29b43a70

After the patch

https://i.imgur.com/ZGc3yqp.jpg

Before the patch

https://i.imgur.com/hztJ9kj.jpg

#16 @Ankit K Gupta
7 months ago

  • Keywords needs-testing removed

#17 @oglekler
7 months ago

  • Keywords commit added

@mukesh27, let's mark it for commit and ping @spacedmonkey as a maintainer :)

#18 @hellofromTonya
7 months ago

  • Keywords commit removed
  • Milestone changed from 6.4 to 6.5

The patch with tests still needs a final review. REmoving the commit keyword until a committer or a REST API maintainer reviews and tests the patch.

The 6.4 Beta 1 release party is starting shortly. I think this one has run out of time. Moving it to 6.5. However, if a review and commit can be done before the release party starts, committer / maintainer please move it back into the 6.4 milestone.

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


3 months ago

#20 @chaion07
3 months ago

Thanks @swissspidy for reporting this. We reviewed this ticket during a recent bug-scrub session. Based on the feedback received we feel that the ticket is closer to being ready for commit. We request @swissspidy or any maintainer to review the PR before committing.

Cheers!

Props to @mukesh27 & @swissspidy

#21 @swissspidy
3 months ago

  • Owner set to swissspidy
  • Resolution set to fixed
  • Status changed from new to closed

In 57380:

REST API: Support assigning terms when creating attachments.

Props mukesh27, Dharm1025, Ankit K Gupta, swissspidy, dharm1025, tanjimtc71, timothyblynjacobs, spacedmonkey.
Fixes #57897.

#24 @stevenlinx
2 months ago

  • Keywords add-to-field-guide added
Note: See TracTickets for help on using tickets.