more detailed notice for register_rest_route

[Lwangaman]

After a few plugins on my website have received updates recently, I started seeing hundreds of these notices on my backend:

Notice: register_rest_route was called incorrectly. Namespace must not start or end with a slash. Please see Debugging in WordPress for more information. (This message was added in version 5.4.2.) in /var/www/vhosts/bibleget.io/httpdocs/wp-includes/functions.php on line 5167

Sure I can try disabling the plugins one at a time to find which one is the culprit, but it would be helpful to know who is trying to call register_rest_route() , it would make life that much easier and I can contact the plugin author to let them know.

Would it be possible to give a little bit more of a stacktrace when giving these notices?

[TimothyBlynJacobs]

Thanks for the ticket @Lwangaman!

Yeah we could add the namespace that errored to the message. Do you want to work on a patch to add that @Lwangaman?

[Lwangaman]

added offending namespaces / routes to _doing_it_wrong error messages

[Lwangaman]

ok @TimothyBlynJacobs I created a simple patch with the offendig namespaces and routes included in each _doing_it_wrong call in the register_rest_route function.

[Lwangaman]

I also fixed a typo in another error message in the rest_sanitize_value_from_schema function while I was at it...

[TimothyBlynJacobs]

Thanks for the patch @Lwangaman!

I think we should make the namespace and route part of the translated error message itself. I don't think I've seen Core using a pattern similar to what you have in your patch. An example to look at would be _wp_scripts_maybe_doing_it_wrong.

[Lwangaman]

updated version from the first patch

[Lwangaman]

ok @TimothyBlynJacobs I updated the patch, is this better? I didn't want to have to make translators re-translate these strings, but I guess it would be cleaner this way.

[TimothyBlynJacobs]

Yep that's better thanks @Lwangaman! Let's also make sure that the sprintf placeholders numbering matches the order they appear in the English string.

In the translators comment we can also do 1. Description instead of the full %1$s placeholder.

[Lwangaman]

The reasoning behind the numbering order of the variables was the fact that logically, namespaces precede routes. So I ordered the sprintf variables logically: you will always find the namespace first and the route second.
However in the error message itself, depending on the wording, sometimes the route might be mentioned before the namespace. That's the whole reason for using numbered variables: you can change the order in the sentence any which way without having to change the order of the variables as they are passed in. So I would really prefer not to change that order, I believe it is more clear to leave the logical ordering namespace->route for the order of the variables as they are passed in.

[TimothyBlynJacobs]

The reasoning behind the numbering order of the variables was the fact that logically, namespaces precede routes. So I ordered the sprintf variables logically: you will always find the namespace first and the route second. However in the error message itself, depending on the wording, sometimes the route might be mentioned before the namespace.

In every error message the route is mentioned before the namespace. I agree its more logical for the namespace to come first when both are necessary. So why don't we order the error message that way?

That's the whole reason for using numbered variables: you can change the order in the sentence any which way without having to change the order of the variables as they are passed in.

Right, but that's primarily used in WordPress so that translators can safely reorder variables.

So I would really prefer not to change that order, I believe it is more clear to leave the logical ordering namespace->route for the order of the variables as they are passed in.

More logical for who? Translators aren't seeing the order of the function arguments, just the original string. I think for them it'll be more confusing for the order to be mismatched.

In functions where translations and sprintf placeholders are used, the order doesn't always match the function signature. For instance, map_meta_cap or rest_handle_deprecated_function.

Throughout the codebase I could only find one example of the %2$s placeholder preceding the first %1$s in the original string, and that is in a ​test case and isn't translated.

I searched for the pattern %2\$s.+%1\$s which wouldn't catch everything, but it seems clear to me that by far the dominant pattern in WordPress is for the parameters to match the order of the English string.

[Lwangaman]

change grammatical order of a couple messages

[Lwangaman]

hey @TimothyBlynJacobs I've tried to take into account your considerations, so I have modified the grammatical order of a couple of the sprintf messages (https://core.trac.wordpress.org/attachment/ticket/50493/register_rest_route-patch-2.patch.txt). I don't believe we can only take into account translators, we must also take into account an orderly code base. Seems to make sense to me that in the code base, the logical order should be respected, so you will always find the namespace before the route.
In the translatable messages themselves, it really depends on what the message is trying to convey. So in those cases where the "route" is the subject, I tried to have it come first in the sentence, and in those cases where the "namespace" is the subject, I tried to have it come first in the sentence. Or in any case, when it was grammatically feasible to have the "namespace" come first, I tried to do so so that the grammatical order would follow the logical order. But that isn't always the case, and I really don't think we should force this. Translators shouldn't really care whether numbered placeholders have a certain order, it's actually self-explanatory from the translator comment provided, where the logical order IS respected.
I'm not sure how this could be done better without becoming confusing in the codebase...

[Lwangaman]

Just to say, this is just my opinion, I'm a plugin developer but I haven't contributed much to the codebase yet. I like to be precise and logical and I like to write clean code, but if you think it really should be done differently don't be afraid to say so, I can change the code accordingly. In my opinion, if we have ten sprintf error messages that all deal with "namespace" and "route", it makes for cleaner code to have them ordered in the same logical order throughout the codebase (considering they're all within the same function, near each other), seems to me to make for better readability. But that's just a personal opinion, if you do think it should be done differently I will change it. I'm just throwing in my two cents. And if you think it's cleaner to put "1. 2." in the translator comments rather than "%1$s %2$s", I can change that too, I'm not as familiar with the current codebase and conventions, so let me know if this is a change worth making, no problem. I see the other translator comments that don't have numbering use %s or %d, if when using numbering "1. 2." seems cleaner that's fine with me.

added namespace and route info to _doing_it_wrong messages to better identify who or what is "doing it wrong"
(cleaner translator strings, using 1. 2. instead of %1$, %2$)

[Lwangaman]

@TimothyBlynJacobs I cleaned up the translator requests and they are now numbered 1. 2. ...
Also, the numbers are all sequential like you suggested except for one string:

/* translators: 1: rest_api_init, 2: string value of the namespace, 3: string value of the route */
__( 'REST API routes must be registered on the %1$s action. Instead route \'%3$s\' with namespace \'%2$s\' was not registered on this action.' ),
'<code>rest_api_init</code>',
'<code>'.$namespace.'</code>',
'<code>'.$route.'</code>'

The subject of the first sentence are the "routes". So it makes sense that in the second sentence, the subject of the sentence is the "route" in question. I don't see a way of switching the order of "route" and "namespace" in the second sentence without changing the meaning or keeping "route" as the subject. And I figure the code is cleaner if the parameters $namespace and $route are passed into the sprintf in the same logical order as all the other messages before it. Is this acceptable? Do you think it can be any cleaner?

removed extra spaces on line endings

[Lwangaman]

I'm seeing that the ​travis job triggered by the ​pull request for code review is failing. Looking into it, it's something about PHP linting.

It says:

PHP CODE SNIFFER VIOLATION SOURCE SUMMARY
----------------------------------------------------------------------
    SOURCE                                                       COUNT
----------------------------------------------------------------------
[x] Squiz.Strings.ConcatenationSpacing.PaddingFound              16
----------------------------------------------------------------------
A TOTAL OF 16 SNIFF VIOLATIONS WERE FOUND IN 1 SOURCE
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SOURCES AUTOMATICALLY (16 VIOLATIONS IN TOTAL)
----------------------------------------------------------------------

I'm not sure what that refers to? I don't see any spacing on any concatenation operators. Unless it's picking it up from the first patch I submitted where I was simply appending the new strings to the previous strings. Other than that I don't see any concatenation spacing. Is there anything that needs to be fixed? Sorry I'm kind of new to these workflows...

trying to fix the failing travis build by *adding* whitespacing to string concatenations instead of removing it

[Lwangaman]

resync with upstream

Replaces PR #422 linked to https://core.trac.wordpress.org/ticket/50493 .

@TimothyBJacobs fixed

[TimothyBlynJacobs]

In 58933:

REST API: Improve error messages when registering an invalid route.

The error messages now include the REST API namespace and route to help identify the offending code.

Props lwangaman, timothyblynjacobs.
Fixes #50493.

Merged in r58933.

Thanks for the PR!