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

[caercam]

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

[adamsilverstein]

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

[caercam]

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.

[adamsilverstein]

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.

[adamsilverstein]

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

[caercam]

Works as expected from what I can tell. Neat!

[euthelup]

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]

Add an extra check

[adamsilverstein]

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

[adamsilverstein]

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

[euthelup]

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

[adamsilverstein]

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

[adamsilverstein]

In 40672.3.diff​:

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

[adamsilverstein]

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.

[Soean]

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

[adamsilverstein]

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

[adamsilverstein]

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?

[adamsilverstein]

40672.5.diff​ cleanup and leave initialize in baseModel

[adamsilverstein]

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.