-
Notifications
You must be signed in to change notification settings - Fork 260
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
Add option to upload languages configuration via API. #2170
Conversation
continue; | ||
} | ||
$lang->setAllowSubmit(true); | ||
if (isset($language['name'])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use a PropertyAccessor loop here to get rid of most duplication. See also the problem import somewhere (I believe)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this and it was not really less code / more understandable (probably also because of the nested compiler/runner fields). Feel free to submit to this branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I’m fine as is
[$idField => $lang_id] | ||
); | ||
if (!$lang) { | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we not indicate this to the one uploading that the language is missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and no:
- Logging is not really possible as we return the updated languages config, so that it can be compared in the script.
- We could error out, but that's quite brutal.
- We could try adding a new language, but of course stuff like executables are missing here.
What about this: I will add a TODO comment here for now; and when we decided on the best solution we can replace it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with that.
Fixes #2142.