Filters are incorrectly applied in the __experimental/menu-items controller

[anton-vlasenko]

Description

This PR fixes an issue with the filters in the WP_REST_Menu_Items_Controller::prepare_item_for_database method.
We explicitly apply title_save_pre, exceprt_save_pre, content_save_pre filters in there.
But later, we apply the same filters in the wp_insert_post function, so we don't need to apply them in the WP_REST_Menu_Items_Controller::prepare_item_for_database method.
Fixes #25095

How has this been tested?

Two regression unit tests have been written to cover this case. So I would say no testing is needed.
However, If you still want/need to test this, you can do it as described below.
Prerequisite: you will need XDebug to test it.

1. Go to the Gutenberg -> Experiments page and enable Navigation screen feature.
2. Save the changes.
3. Open /wp-includes/plugin.php file in your IDE and place a conditional breakpoint at the beginning of the apply_filters function (somewhere around line 166).
4. Use the following condition for your breakpoint:

$hook_name === 'content_save_pre'

5. Go back to your browser, and open the Gutenberg -> Navigation page.
6. Run the following JavaScript code in your browser's console:

wp.apiFetch( {
    path: '/__experimental/menu-items',
    method: 'POST',
    data: { title: 'Some Link', url: 'https://my.test.url' },
} ).then( ( res ) => {
    console.log( res );
} );

7. Make sure that apply_filters function gets called only once (with $hook_name === 'content_save_pre').

Screenshots

No screenshots are needed for this issue.

Types of changes

Bug fix.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• I've tested my changes with keyboard and screen readers.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

[spacedmonkey]

Have you validated we don't need these filters?

I also, think, we do not need to add a unit test here. These menu endpoints are still in development and not stable yet, so removing filters doesn't need a unit test.

[spacedmonkey]

This is copied from here.

Do you know why they are there?

[anton-vlasenko]

Do you know why they are there?

I guess they are there to call apply_filters function before saving values to the database.
But that is not needed in lib/class-wp-rest-menu-items-controller.php file because we apply those filters later (yes, I've checked that, please see How has this been tested? section).

[anton-vlasenko]

Have you validated we don't need these filters?

Yes, I've validated that. Please see How has this been tested?. We do apply the same filters later. I believe we don't want to apply these filters twice.

I also, think, we do not need to add a unit test here. These menu endpoints are still in development and not stable yet, so removing filters doesn't need a unit test.

We don't want to apply the same filters twice, and my unit test ensures that it will not happen again. It's a regression unit test.
If we change something in the endpoint we can always refactor the unit test.
But if we remove the unit test the same bug could appear again undetected.

[spacedmonkey]

We don't want to apply the same filters twice

I was the one who originally added the filter and wrote this whole endpoint. I am happy to remove them. But the unit test is pointless and should be removed. The only way that the filter would be double applied, if someone adds it. Remove the tests, I will approve the PR.

[TimothyBJacobs]

Code changes look good to me, thanks for the fix @anton-vlasenko! I agree the unit tests here aren't strictly necessary, but I don't think there is much of a cost to having them. I'll leave the final decision up to @spacedmonkey as he's been the main driver of this endpoint.

[spacedmonkey]

Can you remove the tests for now @anton-vlasenko.

I have created a breakout issue, #34992 for improving test coverage for these endpoints. If it makes sense, we can add these tests back in that ticket. Agreed?

[anton-vlasenko]

@spacedmonkey

But the unit test is pointless and should be removed

Let me disagree with that. It's a valid regression unit test and it's not pointless.
Having more unit tests doesn't harm anything.
Even a "bad" unit test is better than no unit tests at all.

The only way that the filter would be double applied, if someone adds it.

Yes, and it can happen again.
The probability of this is not zero.
How can someone guarantee it will not happen again in, e.g., four years?
Unit tests are the only way to ensure that.

Can you remove the tests for now @anton-vlasenko.

Yes, I will remove it, but I haven't heard clear arguments against it.
So, hopefully, we can add it back as part of the #34992.