WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 6 weeks ago

Last modified 6 weeks ago

#49749 closed defect (bug) (fixed)

Registering rest routes with a slash-prefixed namespace give inconsistent results

Reported by: skarabeq Owned by: whyisjake
Milestone: 5.4.2 Priority: normal
Severity: normal Version: 5.4
Component: REST API Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

In the custom API when I use the prefix wp, I have got the error message - No route was found matching the URL and request method

{
    "code": "rest_no_route",
    "message": "No route was found matching the URL and request method",
    "data": {
        "status": 404
    }
}

In the last stable WordPress version 5.3.2, it wasn't happening!

Change History (23)

#1 @skarabeq
3 months ago

  • Summary changed from Bug in WordPress API when I have the prefix for wp for the endpoint to Bug in WordPress API when I have the prefix wp for the custom endpoint

#2 @ocean90
3 months ago

  • Component changed from HTTP API to REST API
  • Owner dd32 deleted
  • Severity changed from major to normal

Might be related to #48530.

#3 @TimothyBlynJacobs
3 months ago

Hi @skarabeq,

Can you share your register_rest_route call?

#4 @skarabeq
3 months ago

  • Severity changed from normal to major

Yes, of course

<?php
$model = new Model;
register_rest_route('/wp/v2', '/nav/' ,
    [
        [
            'methods'  => 'POST',
            'callback' => [$model, 'get_data'],
        ],
        'schema' => null,
    ]
);

#5 @TimothyBlynJacobs
3 months ago

  • Keywords 2nd-opinion added
  • Severity changed from major to normal

Thanks @skarabeq,

The issue here is that you are registering your route with an invalid namespace. The namespace must not be prefixed by a forward slash. This configuration works.

<?php
register_rest_route(
        'wp/v2',
        '/nav/',
        array(
                'methods'  => 'GET',
                'callback' => static function () {
                        return new WP_REST_Response( null, 204 );
                },
        )
);

Now, register_rest_route sanitizes this malformed namespace when building the final route, but does not pass the sanitized form to the server.

<?php
$full_route = '/' . trim( $namespace, '/' ) . '/' . trim( $route, '/' );
rest_get_server()->register_route( $namespace, $full_route, $args, $override );

I think this would have resulted in broken behavior previously, but would have been much more subtle. I believe your route wouldn't properly appear in the index.

We could fix this in core by doing something like this so the namespace parameter is forced to be valid. cc: @kadamwhite

<?php
$namespace = trim( $namespace, '/' );
$full_route = '/' . $namespace . '/' . trim( $route, '/' );
rest_get_server()->register_route( $namespace, $full_route, $args, $override );

For now, my recommendation would be to drop the preceding slash.

#6 @afercia
3 months ago

Seems to me the namespace pattern is well documented, right?
https://developer.wordpress.org/rest-api/extending-the-rest-api/adding-custom-endpoints/#namespacing

If the leading slash is confirmed as the cause of the reported issue, I'd say this is not a bug and the /wp/v2 pattern worked in WordPress 5.3.2 only "by accident".

#7 @skarabeq
3 months ago

@afercia, I do not agree with you. I have use this namespace /wp/v2 more than 2 years. So this isn't "accident".
@TimothyBlynJacobs about this code which is in your core:

<?php
$full_route = '/' . trim( $namespace, '/' ) . '/' . trim( $route, '/' );

And your suggested code

<?php
$namespace = trim( $namespace, '/' );
$full_route = '/' . $namespace . '/' . trim( $route, '/' );

The output of the $full_route will be same.

https://imgur.com/CGz2lHM
https://imgur.com/0nGqSCM

#8 @TimothyBlynJacobs
3 months ago

The output of the $full_route will be same.

@skarabeq the output of the full route is the same, but notice that the namespace passed will now be forced to have correct formatting.

I do not agree with you. I have use this namespace /wp/v2 more than 2 years. So this isn't "accident".

I do not believe that prefixing the namespace with a slash is expected behavior, and I agree with @afercia that it is documented fairly clearly.

The existing routing worked because of the coercion that was applied. But this was previously broken in other ways. Any code that relied upon a valid namespace value would be broken. For example, you can see this when doing a request for /wp-json/wp/v2, the nav endpoint won't be listed.

That doesn't mean we won't also add that coercion to the namespace parameter as I described earlier. But I think your best course of action would be to correct your route definition while we wait for 5.4.1.

#9 @skarabeq
3 months ago

@TimothyBlynJacobs , thank you for your replies. Do you know when will be introduced WP 5.4.1?

#10 @TimothyBlynJacobs
3 months ago

I do not. 5.4.1 may be discussed at the dev chat today at 21:00 UTC

#11 @SergeyBiryukov
3 months ago

  • Milestone changed from Awaiting Review to 5.4.1

Moving for 5.4.1 consideration.

#12 @skarabeq
3 months ago

@TimothyBlynJacobs, one more thing about this problem. If there is the namespace which is not named with word wp then I'm able to use the namespace with a slash at the beginning of the namespace, but if there is the namespace - wp and it starts at the beginning with the slash, then I have the described problem. So I think that your solution is just workaround.

#13 @TimothyBlynJacobs
3 months ago

It is not a workaround, a namespace with a prefixed slash is not valid. It is able to serve the routes when you register your own namespace with a prefixed slash because all the routes are belonging to the same group. If you had registered both /my-ns and my-ns as a namespace you'd have the same issue.

And, if you register that route, the index will not work for you.

<?php
add_action('rest_api_init', function() {
        register_rest_route(
                '/lp/v2',
                '/nav/',
                array(
                        'methods'  => 'GET',
                        'callback' => static function () {
                                return new WP_REST_Response( null, 204 );
                        },
                )
        );
});
curl https://trunk.test/wp-json/lp/v2/
{"code":"rest_no_route","message":"No route was found matching the URL and request method","data":{"status":404}}

#14 @TimothyBlynJacobs
3 months ago

  • Summary changed from Bug in WordPress API when I have the prefix wp for the custom endpoint to Registering rest routes with a slash-prefixed namespace give inconsistent results

#15 @skarabeq
3 months ago

  • Summary changed from Registering rest routes with a slash-prefixed namespace give inconsistent results to Bug in WordPress API when I have the prefix wp for the custom endpoint

@TimothyBlynJacobs, Ok, I agree that in the world of the WordPress the route with slash is not valid.. But I can't understand that because the namespace of route with slash and the namespace of the route without slash should be same. If someone make two routes with the same namespace and endpoint, then he is made something wrongly. You can get best practice in the one of the best PHP frameworks - Laravel - Routing - https://laravel.com/docs/7.x/routing.
One of the best pattern of the grouping routes is like this:

<?php
Route::prefix('admin')->group(function () {
    Route::get('navigation', function () {
        // Matches The "/admin/navigation" URL
        return AdminModelNavigation::getAll();
    });
    Route::get('users', function () {
        // Matches The "/admin/users" URL
        return AdminModelUsers::getAll();
    });
});

But, WordPress is far, far away from this path. So do the best, and if you fix the problem that I meet I will be very happy :)

Kind regards,

#16 @TimothyBlynJacobs
3 months ago

  • Summary changed from Bug in WordPress API when I have the prefix wp for the custom endpoint to Registering rest routes with a slash-prefixed namespace give inconsistent results

But I can't understand that because the namespace of route with slash and the namespace of the route without slash should be same.

Yes, as I mentioned earlier, the route is coerced to be the same. But that coercion did not apply to the actual namespace value. The change we'll make is to apply that coercion to the namespace as well.

This will be fixed in 5.4.1, but at the moment that doesn't look at least a few weeks out.

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


3 months ago

#18 @whyisjake
2 months ago

  • Milestone changed from 5.4.1 to 5.4.2

@TimothyBlynJacobs I am going to bump from 5.4.1 since we don't have a patch/testing yet.

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


7 weeks ago

#21 @TimothyBlynJacobs
7 weeks ago

  • Keywords has-patch has-unit-tests added; needs-patch 2nd-opinion removed

I've uploaded a PR that issues a _doing_it_wrong notice and cleans the namespace to be valid.

#22 @whyisjake
6 weeks ago

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

In 47842:

REST API: Ensure proper namespacing when registering routes.

The PR will corerce routes that have a leading slash and throwing a _doing_it_wrong notice while ensuring a proper namespace.

Fixes #49749.
Props TimothyBlynJacobs, skarabeq, afercia.

#23 @whyisjake
6 weeks ago

In 47843:

REST API: Ensure proper namespacing when registering routes.
The PR will corerce routes that have a leading slash and throwing a _doing_it_wrong notice while ensuring a proper namespace.

This brings the changes from [47842] to the 5.4 branch.

Fixes #49749.
Props TimothyBlynJacobs, skarabeq, afercia, skithund.

Note: See TracTickets for help on using tickets.