Skip to content
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

Move locale files from language pack to core #2408

Merged
merged 5 commits into from Feb 17, 2021

Conversation

rob006
Copy link
Contributor

@rob006 rob006 commented Oct 24, 2020

@rob006 rob006 force-pushed the move-locale-files-to-extensions branch from 2c1f6b3 to 8f09b74 Compare October 24, 2020 19:28
content: content
name_singular: singular name
name_plural: plural name
tag_count_primary: number of primary tags
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way the tag-specific stuff could remain with tags?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it matters where these strings are located. It may only be a problem if some extensions use these strings and tags extension is not enabled.

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As of right now, the only parts of core that are aware that tags even exists are:

  • The installer, which enables bundled extensions
  • Some examples in comments

I'd like to avoid expanding that to keep the division between core and extensions as strong as possible. I don't remember off the top of my head, but if we have 2 validation.yml files, one in core, the other in tags, would they be merged by the translator? If so, we should do that; if not, we may want to investigate supporting that.

Copy link
Contributor Author

@rob006 rob006 Jan 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For extension you need to put these translations in en.yml together with the rest of translations for this extension. They should be merged in the same as any other translation.

BTW: Are you sure that this is still used? I can't find any occurrences of these keys on https://github.com/flarum/tags.

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that makes sense. Let's go for that if it works!

BTW: Are you sure that this is still used? I can't find any occurrences of these keys on https://github.com/flarum/tags.

Validation translations aren't referenced directly, but rather used in Laravel validation error messages: If a discussion is submitted with too few primary or secondary tags, these error messages will be used.

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, looks like we found a bug!

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Written up at https://github.com/flarum/core/issues/2572. Regardless, I think tag's validation translations should live in the tags repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could remove them here, but IMO this should be done in separate PR.

BTW: I'm not sure if we should use validation.attributes.name keys outside of validation.yml. It will encourage extensions developers to do the same, and this is straight way to collisions.

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could remove them here, but IMO this should be done in separate PR.

When all extensions were in a single folder, it made sense to have a shared file for validation logic separated out, but since it's being split, tag-specific validation errors and attribute names should be located in the tags repo.

BTW: I'm not sure if we should use validation.attributes.name keys outside of validation.yml. It will encourage extensions developers to do the same, and this is straight way to collisions.

Why would developers need to override that per-extension, and not just reply on the ones from core? I'm envisioning tag's validation translations being just those 2 attribute names.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When all extensions were in a single folder, it made sense to have a shared file for validation logic separated out, but since it's being split, tag-specific validation errors and attribute names should be located in the tags repo.

I'm not sure how do you want to proceed with this. As I said, I can remove these keys in this PR. But I don't want to block this PR and wait another X months until you decide where these lines should be and what keys they should use (especially as they don't work anyway). Merging this as is and having 2 unnecessary keys should not be a big deal - it could be fixed later as part of #2572.

Why would developers need to override that per-extension, and not just reply on the ones from core?

I'm not talking about overwriting core translations, I'm talking about collisions as a result of using the same namespace (validation.attributes.) for attributes names of extensions (because official extensions will do this, everyone can copy this pattern) - key validation.attributes.name could be used by two separate extensions unaware each other, and they may need to be translated differently depending on context (for example in Polish "name" could be translated either as "imię" or "nazwa"). To avoid this it should be something like flarum-tags.validation.attributes.name, but I'm not sure how problematic this would be with Laravel validator (it looks like black magic for me :/).

@rob006 rob006 marked this pull request as draft February 17, 2021 15:26
@rob006 rob006 marked this pull request as ready for review February 17, 2021 20:15
@rob006 rob006 force-pushed the move-locale-files-to-extensions branch from f13f64d to d159a9c Compare February 17, 2021 20:28
@askvortsov1 askvortsov1 merged commit c4ebebe into flarum:master Feb 17, 2021
@rob006 rob006 deleted the move-locale-files-to-extensions branch February 17, 2021 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants