POC - Localized errors

[shmax]

Proof of concept for alternative fix to #713

I didn't want to complete the work until I get some positive feedback, so for now I just converted two errors.

Features:

• backwards compatible. No major bump required.
• All error strings are centralized in one place
• No new public API properties exposed; everything is done with the extensions entry on Error already provided by the graphql spec
• errors can be localized on front or back end
• addition of error codes allows client code to do logical branching as desired when errors occur

Notes:

• I didn't bring in the enum class like I did the last time I implemented something like this because I figure we'll already have enough to talk about without it, but the option is always there.
• The ErrorCode as first argument to Error might be a bit awkward, but the extensions argument is currently the seventh argument on the Error constructor and I didn't want to make a bigger mess than I had to for right now. If we go all-in on something like this we can rearrange Error to be cleaner.

That's the general idea. I expect you guys to have lots of feedback, and I'm ready to do my best to address it.

[coveralls]

Coverage increased (+0.02%) to 86.329% when pulling e848bec on shmax:localized-errors into 4f34309 on webonyx:master.

[spawnia]

How does that allow the errors to be localized?

[vladar]

Nice! One problem is that the error message is not directly reachable from here anymore. So I have to go to ERR_CANT_SPREAD_FRAGMENT constant and then literally search for its usage to reach the actual error message.

That's what I meant when mentioned that it will make maintenance and syncing with upstream graphql-js less convenient.

[shmax]

So I have to go to ERR_CANT_SPREAD_FRAGMENT constant and then literally search for its usage to reach the actual error message

I don't understand what you mean. If you're already at this bit of code you can just ctrl+click to jump to the error code (well, in Intellij, anyway) definition. And if you're at the error code and want to see where it's used, yes, you can search for it (or in Intelli, "Find Usages"). And having the strings centralized like this also means that we can reuse them (though I haven't checked how much value we would get out of that). If this is going to be a blocker then I can see if I can work out some scheme for each class to manage its own strings, but that's a lot of complexity for the sake of a little convenience and I'm not going to bother unless you're going to insist on it.

[vladar]

Replying on #713 (comment)

This approach does not allow client code (meaning javascript) to do the localization. Mine does.

This is not something we want in this library. I explained why in the linked issue: those errors are for app-developers not for end-users. Most of the apps have their own tools for app-level errors and localizations (which better suit their specific needs).

This approach exposes 50+ new public API properties. Mine adds one.

One public property and 50+ constants which is no different in terms of maintenance. And in fact, it exposes even more: all the new keys in extensions - we'll have even stricter commitments to maintain them since they are now a part of the public JSON response.

I think this is something we should specifically avoid in this lib. As extensions is not for the library that implements a specification. We should only cover things explicitly described in specs.

Maybe someday they will add error localization to the spec and we will be in a weird position.

I've already made a mistake in the past by adding a category in extensions. Now we can't change it easily as many apps rely on it.

I agree that your solution to localization is nice. But it adds a feature we shouldn't have in this library.

[shmax]

How does that allow the errors to be localized?

Well, each error now comes with an "extensions" bucket, and in there you will find an error code, a reference error message (suitable to pass directly to sprintf if you don't have your own localized version of the string). Somewhere in your consuming code (either back end or front end), you execute your query and wind up with a result, right? You check the result to see if there are any errors, iterate over them, check each error code, and pass your own localized string along with the provided args to sprintf (which I know you hate, sorry).

[shmax]

This is not something we want in this library. I explained why in the linked issue: those errors are for app-developers not for end-users. Most of the apps have their own tools for app-level errors and localizations (which better suit their specific needs).

I don't understand. You say you don't want it in this library, but you are considering merging the other PR, which has the same general goal as my PR. Which bit is it you don't want in this library?

One public property and 50+ constants which is no different in terms of maintenance. And in fact, it exposes even more: all the new keys in extensions - we'll have even stricter commitments to maintain them since they are now a part of the public JSON response.

And in fact, it exposes even more: all the new keys in extensions

How does it expose even more? The other PR adds 50+ public properties to the class hierarchy. Once they're out there you can no longer rename or remove them. I added 3 properties to the extensions bucket. 50 is more than 3, is it not?

we'll have even stricter commitments to maintain them since they are now a part of the public JSON response.

Yes, and with the other PR you have strict commitments to maintain 50+ public properties on the native code as part of the public API. Either way the API has changed, but with my approach the only thing exposed are those little properties on extensions.

Now, I'll admit that I'm not entirely clear on what the intended purpose of "extensions" is. The spec says this:

GraphQL services may provide an additional entry to errors with key extensions. This entry, if set, must have a map as its value. This entry is reserved for implementors to add additional information to errors however they see fit, and there are no additional restrictions on its contents.

Maybe you can help me understand what this means. What do they mean by "services"? What do they mean by "implementors"? If we are the implementors they are speaking of, then I seem to be doing things the way they hint at (notice the "code" property in their sample).

As extensions is not for the library that implements a specification.

Well, that's interesting. What does it mean, exactly? Who is it for?

I agree that your solution to localization is nice. But it adds a feature we shouldn't have in this library.

Let's be clear, here. Is the feature you don't want localization? Or is it the modification to "extensions" you don't want? Because there's still all kinds of room for other ideas, here. If you really want to keep all this on the PHP side of things, then we could tweak this a bit and do something similar to type loader, where the client PHP code can register an "error loader" callback. It would receive the error code and the args, and they would return the localized error message. You get the same functionality, but a smaller API change than either of these PRs. I'd be just as happy with that if you would prefer it.

[spawnia]

we could tweak this a bit and do something similar to type loader, where the client PHP code can register an "error loader" callback. It would receive the error code and the args, and they would return the localized error message

I like this option the best so far, since it gives a lot of power while staying relatively simple.

$translations = [
    ErrorCode::SOME_CODE => 'Meine übersetzte Nachricht mit %s',
];

Error::registerErrorCodeHandler(function (ErrorCode $code) use ($translations): string {
    $translated = $translations[$code->code];
    return sprintf($translated, ...$code->args);
});

[shmax]

I like this option the best so far, since it gives a lot of power while staying relatively simple.

Works for me, then. I'll do a new PR after work (or maybe modify this one). Thanks for the feedback.

[shmax]

I had to work late, but I did start trying to research some of this. I discovered that we already have the concept of an errorFormatter, which seems promising. Maybe this could all be as simple as doing everything through the error formatter API we already have (there are errorFormatter members on both ExecutionResult and ServerConfig). Unfortunately, I didn't get as far as figuring out how it works, and we don't really seem to have any tests that demonstrate its usage (unless I missed something). All I could find was stuff like this:

https://github.com/webonyx/graphql-php/blob/master/tests/Server/ServerConfigTest.php#L80

I'll keep digging tomorrow, but I'd appreciate any insight you guys can provide...

[vladar]

I don't understand. You say you don't want it in this library, but you are considering merging the other PR, which has the same general goal as my PR. Which bit is it you don't want in this library?

What I don't want here:

1. Client-side localization via extensions
2. Error codes

The other PR doesn't include client-side localization. It merely exposes a way to replace error messages on the server. It doesn't alter the shape of the public response. Also, it will be easier to maintain and sync with upstream graphql-js.

As for error codes - they are not a part of the spec yet. But can become someday. This will likely introduce a conflict for this library. I think the correct way to introduce error codes is to propose them in spec.

How does it expose even more? The other PR adds 50+ public properties to the class hierarchy. Once they're out there you can no longer rename or remove them.

The same is true for 50+ constants in the ErrorCode class, right? It is also a public API.

Let's be clear, here. Is the feature you don't want localization? Or is it the modification to "extensions" you don't want?

So to re-iterate, the scope of this library is GraphQL specification. We shouldn't include opinionated features here. There is a way to customize the shape of errors and that's where anyone can add their custom extensions if they want.

Most APIs already do this. They have their own error formatting which will be in conflict with ours if we introduce one.

[shmax]

Client-side localization via extensions

Okay, fine.

Error codes

Wait, what's wrong with error codes? As long as we're not changing the public response and it's just something we do internally, what's the harm? If we use some kind of key then we can funnel all of the localization through a single point and you don't have to add 50+ public setters.

And by the way, all those setters are static, which is a very clumsy way to do things. I have a feeling you're going to hand-wave it all away because you're having fun busting my chops, here, but when you use static properties you undermine the instance-based nature of the rest of our config system. Sure, the guy that opened that PR won't care because his use case is satisfied, but imagine a situation where someone wants to, I dunno, switch localization on and off for some reason; with static setters you can't really do it without making a big mess. I mean, how would you unit test the static method approach? I notice there are no tests in that other PR. Your test would have to set the static properties on a few classes to make sure it works, right? Okay, but unless the tests run in exactly the right order, wouldn't it break the other tests?

It doesn't alter the shape of the public response.

Sure, I can live with that, fine. Agreed.

Also, it will be easier to maintain and sync with upstream graphql-js.

You're going to have to be a bit more clear about that. If your idea here is that you want the default error strings to remain inline, then I think I can accommodate that, but I would still need to have some kind of identifier to pass to a transform callback.

So, to summarize:

1. You don't want to alter the public response. Okay, I can deal with that.
2. You want to keep the default error messages inline. I think we can do that, too (though it will make the task of actually doing localization more difficult, because the person doing the localization will have to chase around through the code base trying to find every single error string--we don't seem to have any strong convention for identifying them at the moment).
3. You don't want error codes. Well, what's an error code? In PHP Error terms, it's actually an int. I'm more interested in a human-readable identifier--let's call it a message id--so that we can channel all localization through a single point. If we keep it all internal and don't expose it to the output in any way, is it still an error code? In localization there is always a message id. Always. I deal with it in my job every day, and have for the past 20 years. You don't want codes in your code base, but think about the poor team doing the localization; they might have a dozen different languages to work with. So you have every string multiplied by 12. To manage all those strings, there's generally a database somewhere and other tooling for managing the strings. This is all possible because there is a message id for every string. Now, imagine trying to manage all this with the 50+ static setters design. Nothing is impossible, but that scenario comes very close.
4. If you don't channel it all through a single point, you get what we have in the other PR, which is 50+ static setters. Static setters are bad, as they are not instance safe, and can't be undone without adding even more complexity.

I'm going to suspend work on this until we can come to an agreement on point 3, because without some kind of message id on errors I can't centralize anything, and I argue that actual localization on a non-trivial scale without it is basically impossible. Think it over. I'll be ready.

[shmax]

Oh, I should mention that PHP actually does have a built-in mechanism for doing localization. There's a method called gettext, which sort of ties into a larger ecosystem of tooling. I had to deal with it a few years back when I was still a web developer. You might like it better because while there are still message ids, they are created automatically and only surface in generated PO (Portable Object) files, so your strings remain inline. The catch is that if you ever change any of your inline strings, then that entry in the PO files is invalidated, and the localizers have to address the change you made (which makes sense, if the meaning has changed). This approach might require our poor localizer to do a bit more set up than he would with either of the other two approaches we're discussing, but it would make our end pretty trivial. Just something to think about.

[spawnia]

I think sprintf based translation is fundamentally flawed, sentence structures can differ between languages. I think error message generation has to go through functions, given the dynamic nature of some error messages.