WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 23 months ago

#40672 closed defect (bug) (fixed)

Undefined property `requireForceForDelete` prevents deleting terms in Backbone.js client

Reported by: caercam Owned by: adamsilverstein
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.7.4
Component: REST API Keywords: needs-patch
Focuses: javascript, rest-api Cc:

Description

The REST API Backbone.js client makes it possible to delete terms using Backbone Model's destroy() function. However, when it comes to models that can't be trashed (like Category and Tag), an additional force=true URL parameter is required to avoid a rest_trash_not_supported error. When the Models are created from endpoints in the constructFromSchema() function, a requireForceForDelete property is set to true to have the required force parameter later added to the URL.

Problem is, the requireForceForDelete property is only set for Models that have a parent in their route, overriding the Model's default wp.api.WPApiBaseModel.initialize() function; if the Model does not have a parent, the default wp.api.WPApiBaseModel.initialize() function is used, requireForceForDelete is undefined, and the DELETE API request fails because of the missing force parameter.

Not sure exactly why there's a difference between Models based on whether or not they have parents, but the end result is that it's not possible to properly delete terms without having the requireForceForDelete added to the Model before calling destroy().

Attachments (6)

40672.diff (2.6 KB) - added by adamsilverstein 2 years ago.
40672.b.diff (2.7 KB) - added by euthelup 2 years ago.
Add an extra check
40672.2.diff (4.2 KB) - added by adamsilverstein 2 years ago.
40672.3.diff (3.8 KB) - added by adamsilverstein 2 years ago.
40672.4.diff (2.5 KB) - added by adamsilverstein 23 months ago.
40672.5.diff (1.5 KB) - added by adamsilverstein 23 months ago.

Download all attachments as: .zip

Change History (32)

#1 @adamsilverstein
2 years ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

@caercam Thanks for the bug report and detailed description.

To clarify, when you try to delete a Category or Tag model you can a failure response from the api? can you provide some code snippets demonstrating what you are trying to do? I will look into why the has parent distinction is in place for the requireForceForDelete property. This should be based on whether the endpoint supports the trash, right?

#2 @caercam
2 years ago

To clarify, when you try to delete a Category or Tag model you can a failure response from the api?

Exactly.

Can you provide some code snippets demonstrating what you are trying to do?

var category = new wp.api.models.Category( { id : 123 } );
category.destroy();

Provided that you have the wp-api script loaded -- and the category 123 exists, or you'll normally get a 404 -- the API response will be:

{
    code: "rest_trash_not_supported",
    data: {
        status: 501
    },
    message: "Terms do not support trashing. Set force=true to delete."
}

I will look into why the has parent distinction is in place for the requireForceForDelete property. This should be based on whether the endpoint supports the trash, right?

Makes sense to me. For now that's kind of hard-coded in wp-api.js -- see https://core.trac.wordpress.org/browser/trunk/src/wp-includes/js/wp-api.js#L1213 -- so the only Models having the requireForceForDelete property defined are PostRevision, PageRevision and UsersMe. The other Models that support trashing shouldn't be concerned, but Category and Tag are made impossible to delete, unless you add the requireForceForDelete property yourself before calling destroy().

Edit: it should be noted that the model you're trying to delete will be removed from every collection it's been part of, no matter the API response. That can become annoying if you have views relying on those collections.

Last edited 2 years ago by caercam (previous) (diff)

#3 @adamsilverstein
2 years ago

  • Milestone changed from Awaiting Review to 4.8

#4 follow-up: @adamsilverstein
2 years ago

  • Keywords has-patch reporter-feedback needs-unit-tests added

In 40672.diff:

  • base addition of ?force=delete when destroying a model on the schema route method data
  • apply force=delete to models with and without parents

@caercam can you please give this a test, in my testing Category models now get ?force=delete, while Post models do not. Some unit test for the expected behavior would be a good addition, I don't think we test for this yet.

#5 @adamsilverstein
2 years ago

  • Milestone changed from 4.8 to 4.8.1

This still needs some unit tests validating behavior before we can merge.

#6 in reply to: ↑ 4 @caercam
2 years ago

Works as expected from what I can tell. Neat!

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


2 years ago

#8 @adamsilverstein
2 years ago

  • Keywords reporter-feedback removed
  • Milestone changed from 4.8.1 to 4.9

This ticket was mentioned in Slack in #core-restapi by adamsilverstein. View the logs.


2 years ago

This ticket was mentioned in Slack in #core-restapi by adamsilverstein. View the logs.


2 years ago

#11 @euthelup
2 years ago

Hi,

I've tried to write some unit tests for this case (and failed miserably) but I noticed that if I apply the patch made by @adamsilverstein the Qunit tests fail at wpapi: Checking WidgetsArchives class name. since the patch at line 1248 doesn't check for the force property.

I'm submitting a patch with a version that works on my end. I hope it makes sense.

@euthelup
2 years ago

Add an extra check

#12 @adamsilverstein
2 years ago

Thanks for taking a look and testing @euthelup, i'll take a look and let you know what I find.

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


2 years ago

#14 @adamsilverstein
2 years ago

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

@euthelup thanks for tracking that test issue down.

In 40672.2.diff I added some unit tests. According to my test, Category, PageRevision, PostRevision, Tag and User all get the requireForceForDelete flag, the remaining tested endpoints do not get it. These should be the endpoints that support the DELETE method and don't support the trash.

I'd like to get this in for 4.9, does this test look good?

#15 @euthelup
2 years ago

I was just about to submit a test case when you added the patch, and I'm thrilled to see that I was about to send something similar(but not so elegant as you did tho, so I bow down now).

Anyway, I just applied 40672.2.diff and after I've run the tests everything is green. From my point of view, this ticket is solved.

If I can ask a trivia question, why the Settings model would not support this? I think it could be useful if there isn't an alternative for it.

Thanks

#16 @adamsilverstein
2 years ago

@euthelup Thanks for testing again, I appreciate the extra eyes/review. The settings endpoint does not support the DELETE verb - you can change settings (PUT), but not delete the settings themselves.

#17 @adamsilverstein
2 years ago

  • Keywords commit added

In 40672.3.diff:

  • Clean up whitespace/error msg on failure in tests
  • move initialize functionality to base model, tests still pass

#18 @adamsilverstein
2 years ago

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

In 41657:

REST API JavaScript Client: improve support for model deletion/trash.

Update the way and location the JavaScript client determines which models/endpoints require the force=true parameter when being deleted to avoid a rest_trash_not_supported error. Identify models with endpoints that support DELETE, excluding those that support the trash (posts and pages by default). Also, move the check into the default wp.api.WPApiBaseModel.initialize() function.

Props caercam, euthelup.
Fixes #40672.

#19 @Soean
2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@adamsilverstein
The following condition doesn't work for localized versions.

'Whether to bypass trash and force deletion.' !== endpoint.args.force.description

The german string is Ob der Papierkorb umgangen werden soll und das Löschen erzwungen. If you delete a post in Gutenberg (WordPress/German language), it gets deleted without trash. In Gutenberg (WordPress/English language) the post is moved to trash.

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


23 months ago

#21 @obenland
23 months ago

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

#22 @adamsilverstein
23 months ago

Ah, good point @Soean I'll work on fixing this.

This ticket was mentioned in Slack in #core-restapi by westonruter. View the logs.


23 months ago

#24 @adamsilverstein
23 months ago

In 40672.4.diff

  • Revert to using a hard coded list of object types that support trash (other types require ?force=true to delete)
  • Test verifying correct object types all pass

Until we have a reliable way to determine if an object type supports the trash, enumerate the types them so we know when to use force=true when deleting.

Ideally we would add information into the schema that the client could use to reliably determine if an endpoint supports the trash on DELETE. @rmccue - where do you think we could expose that information?

Last edited 23 months ago by adamsilverstein (previous) (diff)

#25 @adamsilverstein
23 months ago

40672.5.diff cleanup and leave initialize in baseModel

#26 @adamsilverstein
23 months ago

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

In 42047:

REST API: JS client - Enumerate endpoints supporting trash.

Improve the logic determining which endpoints support the trash by enumerating them. Endpoints that don't support the trash require force=true when deleting. The previous approach relied on the force argument description, which is a translated string and was fragile. In the future, we can expose whether an endpoint supports the trash as part of its schema and automate this logic.

Props Soean.
Fixes #40672.

Note: See TracTickets for help on using tickets.