-
Notifications
You must be signed in to change notification settings - Fork 142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Search extensions fail validation if search parameter values are localized messages #3911
Comments
This is indeed a bug in the linter. These properties are declared as localizable: https://searchfox.org/mozilla-central/rev/d10188f9b4f1c4974264f3925505a0498d346c57/browser/components/extensions/schemas/chrome_settings_overrides.json#49,70,89,127 The correct thing to do here is to enumerate all translations and check that the translations match the specified format |
Well there doesn't seem to be any value I can provide for the translated values that doesn't cause the same error on AMO. I haven't checked every one because some of these extensions have hundreds of translation files. Here's another example that AMO rejects:
I don't think there's any other valid way I can express those url patterns anyway. But like I mentioned in the first post, those same values work perfectly fine if I write them directly in manifest.json. So far the problem only seems to arise when I write them in l10n files and reference them as messages. Idk if that's what you meant. Like, is this pattern format only used to check URL values if they are translated messages? That wouldn't make any sense in my mind. If I had to guess based on this behavior, where the URLs work in manifest.json but don't work in localized messages, I would assume the linter is just literally parsing the URLs as a string value |
Apparently nobody tried it before, or didn't bother with reporting an issue.
It's not possible until the linter supports it. In my previous comment I sketched what the linter should do, patches are welcome if you're interested in unblocking this quickly. |
Is there some other way to squeeze this through the linter? Is it possible to, like, register the search engine dynamically rather than through the manifest? I forgot about the issue and just wasted another day making another search extension with 80 languages lol. Now I have 6 extensions waiting around to publish since I can't seem to get them signed without removing the l10n support. I don't want to force english onto everyone since it defeats the purpose of most of these extensions, but I don't know of any other way to register a localized search engine than to use |
There is no work-around available. Given your (JS) skills and motivation, the fastest way to achieve your goal is to contribute a patch to the linter that accepts the format. Two months ago I described how the patch would look like ;) |
I understand what you said but it's vague and I have no prior familiarity with this project, so I don't think it's at all likely I will figure this out on my own. I cloned the repo but I can't even follow the test's code path from beginning to end. I think I see where it begins, by searching for "should match pattern" and I think I understand the basic structure of the project. But as far as I can tell, all the locale messages are evaluated separately, agnostic to what they're being used for, and the manifest is evaluated separately, agnostic to the values or types of the messages it references. Actually I get the impression it wasn't expected that locale messages would have any other type than string? I'm probably missing something, but I don't see any existing methods for enumerating all matching locale messages and iterating over them with a parser. Instead it seems like LocaleMessagesJSONParser is oriented toward parsing an entire file all at once, not property by property. But this is a huge project and it hurts my brain just trying to find what any one function does, so I probably overlooked something if not several things. I don't really have substantial programming skills. I am just in a very early state of learning javascript. This linter is far beyond anything I've ever worked on in terms of complexity. I mean, I welcome the opportunity to learn more but I don't really have the faintest clue of where to start. Normally I learn by imitation of something concrete, and I can't see anything in the linter that does anything comparable to this. I.e., validating a given property's localized value by validating all the matching locale messages according to the property's |
Admittedly, my previous comment was very concise and didn't have any details on relevant locations in the addon-linter's source code. So I'll try to add some more detail. Feel free to ask follow-up questions (or stop working on this issue). In #3911 (comment), I linked to Firefox source code that shows the definition of the schema. The schema has the To solve this, we would have to find the relevant localized string when needed (i.e. when There are a few possible approaches to do that:
|
So the |
Not in the addons-linter. That's why we have this issue.
This is a link to Firefox's source code, not the linter. It's not necessary to read Firefox's source code to work on this patch (only the schemas, but those are regularly imported into this repository), merely an understanding of the localization feature is sufficient. Since you authored a localized extension, it's clear that you have at least some basic understanding of (but if you are interested in getting a better understanding of Firefox's source code, or even contributing to it, then I'm willing to assist)
I'll link source code below (and already did in my last comment), but it may be easier to understand if you were to add some The addons-linter/src/schema/validator.js Lines 208 to 220 in 93b1d7e
We use ajv version 6.x, whose source code is available at https://github.com/ajv-validator/ajv/blob/v6.12.6/lib/ajv.js. If you aren't able to get the feature to work via the ajv library, then an alternative way to solve the issue is to "manually" preprocess the manifest, before anything is passed to |
Yeah that makes sense. Thanks. I understand that the module I linked is unrelated to the linter, I was just wondering if it was significant that I couldn't find the preprocess property anywhere else. I guess it was just intended for future expansion or something. Actually I am pretty familiar with firefox's frontend code, but I have very little experience with node, so this is all new to me.
My first instinct would be to create some one-off logic for localized properties in the manifest, the way you described, since I don't need to learn a new library to do that. But that would add a lot of redundancy, especially if I intend to validate every message in every language (which seems like it would be mandatory for my patch to be accepted, I imagine it may be possible to abuse otherwise), and I think I get how it works for the deprecated property, so I'm trying to add a preprocess keyword to
But when I try this for example, I get the console messages but the localized URL/component properties still get flagged as errors, presumably by something else. So I guess even if I make this validator function correctly, I'm still missing something crucial. Edit: to be clear, I'm not sure if this is because 1) the properties with Aside from that, in principle I can make whatever translations need to be made for that particular property inside that validator function and only return true if every translation passes the relevant test. And I'm pretty sure by imitating the other source code, I can figure out how to retrieve schema information and all the localized messages for the property being validated. I'm just not really sure how to call the pattern testing functions from within a keyword validator function. The existing keywords in validator.js have much simpler validator functions than what is needed here, and they don't seem to perform any string test anyway. I will need to retrieve the localized messages, then for each message, perform the same pattern/format validation that would otherwise have been applied to the |
Just to satisfy your curiosity:
That's probably because the validator runs the validation logic for every defined keyword. Even if you define a custom keyword, then the default logic for I'm not sure if it is possible to disable validation of other properties. |
What do you mean by the modifier? I thought I could only return booleans. I can use a keyword validator to change what is passed to the ajv pattern validator? |
Point 4 of #3911 (comment). It's not just the brief answer there, but also the references to Github issues/comments at https://stackoverflow.com/questions/50876024/modify-data-using-ajv-for-json-schema#comments-50894529 For more documentation, see this section (which documents the feature for the ajv version that we are using): https://github.com/ajv-validator/ajv/tree/v6.12.6#user-content-defining-custom-keywords |
This issue has been automatically marked as stale because it has not had recent activity. If you think this bug should stay open, please comment on the issue with further details. Thank you for your contributions. |
I uploaded some extensions to the add-on dev hub as normal. I'll just discuss one example for simplicity's sake.
This extension's manifest is very similar to Firefox's built-in search extensions, because my intention was to replace the built-in engines with my own engines where I can control the icon. Some of the built-in engines are fine and have x-suggestions URLs (like Amazon) though others lack suggestions when an API is actually available. I've been meaning to file a bug about this on Firefox but it slipped my mind since I don't know what component to file it under.
I also wanted to replace the icons so they look consistent with built-in icons. That is, replace colorful ICO images with SVG files with context properties, so the icon can respond to themes.
Anyway, here is the form of my version of the Amazon extension. As you can see it's nearly identical to the built-in extension.
And here are some of those localized messages:
Again, you can see how they match the built-in extension's messages.
And here are my results after uploading to AMO developer hub:
Please correct me if I'm mistaken and this is just an issue with my syntax. But I have used the same syntax before with no problems, and for most of these search engines I just copied the l10n files from Firefox's source code. It seems to only become a problem when I try to define a search parameter with a l10n message.
It seems like the linter isn't capable of interpreting the localization syntax and finding message values? Idk. Anyway, I made 4 of these search extensions. I don't really need the extensions to be signed, it's not a big deal. But it would be nice to not have to tell my users to disable the signing requirement, and put up with the alarming "unverified" message in about:addons.
Of course one way or another, if they're using my search extensions, they're already messing with their Firefox profile or installation pretty heavily. But I think this might be a bigger issue beyond just my applications, because shouldn't there be lots of search extensions with localized search parameters? If I want to provide search suggestions in the user's language, declaring one
suggest_url
is not sufficient. I can't think of any way to deliver localized suggestions without this feature, since it's not like there's any infrastructure set up for dispatching or receiving l10n information with the initial request. (to my knowledge)Thanks 🙏
┆Issue is synchronized with this Jira Task
The text was updated successfully, but these errors were encountered: