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

Create view helper for translating EDS labels #2642

Closed
wants to merge 8 commits into from

Conversation

demiankatz
Copy link
Member

@demiankatz demiankatz commented Dec 1, 2022

This is a spin-off from #2598, creating a mechanism for translating EDS labels using a separate text domain.

TODO

  • Replace custom view helper with generic functionality in the standard helper; see Work around illegal characters in translation keys. #3461 for part 1 of this work and Implement text domain fallback in translator. #3471 for part 2.
  • Review existing language files for more strings that should be moved into the EDS domain (possibly identifiable through Git history) and determine whether some of the strings that have been moved should also be deleted from the default domain (many should remain because they are shared, but some may be EDS-specific and eligible for cleanup).
  • Do more EDS testing to identify additional missing strings

@demiankatz demiankatz added this to the 10.0 milestone May 2, 2023
EreMaijala pushed a commit to EreMaijala/vufind that referenced this pull request Aug 11, 2023
…erly check for PDF-images (vufind-org#2642)

Ead3 contains an array of booleans instead of a single boolean
@demiankatz demiankatz added the small Minor changes to relatively few files label Nov 7, 2023
@demiankatz
Copy link
Member Author

demiankatz commented Feb 29, 2024

This PR has been open for a long time, and the original reason for its creation (elimination of illegal translation keys for EBSCO labels) turned out to be a non-issue, so there isn't really a compelling reason to merge it.

However, there are at least two ideas here that might be worth implementing somehow, though I'm fairly convinced that the current helper is not useful as written.

Idea 1: we should prevent the translator from trying to look up illegal translation keys (i.e. keys containing characters that Lokalise will not allow us to support). Should we add key normalization to the standard TranslatorAwareTrait? If so, we probably want something less aggressive than what is implemented here, but maybe we could start by doing something with the four problem characters identified in #2598: ?!(). This might introduce BC problems for people using local custom strings containing those characters, but it would prevent us from accidentally introducing new bad strings in the future.

Idea 2: there are situations where it's useful to have text domain fallback -- i.e. if there is no translation found in domain X, try again in domain Y. The most common reason for this is refactoring of language files (e.g. we've moved some strings out of default and into its own domain, but we want to be sure anything custom or missed still gets translated). There might also be situations where context-sensitive optional overrides would be useful, and this could help. Do we need to create a new helper for this, or might this also be something we should add to the standard translation mechanism? With PHP named arguments, it becomes easier to add some of these optional features without making the translate calls absurd, but maybe it's better to keep this one separate.

I'm interested in opinions on how to move this forward. I'm also open to closing it if nobody thinks this is worth the effort, but since I already did some work here to clean up EDS translations, it would be nice to find a way to actually utilize that.

Any thoughts from @EreMaijala, @ThoWagen or others?

@ThoWagen
Copy link
Contributor

Regarding Idea 1: I don't think that normalizing !?() should big problem for us. If the php-cs-fixer would take care of that the BC problems should be quite easy to fix. But I don't know if there are situations or setups where this change would introduce bigger problems.

Regarding Idea 2: I think that would be a good feature not only for refactoring translations. It should work similar to the translation aliasing but for whole domains. In my opinion that should be part of the standard translation mechanism using named arguments as you said.

@demiankatz
Copy link
Member Author

Thanks, @ThoWagen -- unless others object or have different ideas, I'll see if I can open separate PRs for each of these ideas; once those are individually reviewed and merged, I'll revise this PR to utilize the new mechanisms.

@demiankatz
Copy link
Member Author

On closer inspection, I don't really see a strong justification for creating an EDS-specific text domain. All of the strings being used here are not EDS-specific and may be useful to label values in other contexts. If we're concerned about redundancy, we can address that with aliases as needed. For now, I'm just going to close this without further action. I think the work we've done to improve the translation system gives us the necessary flexibility for the future, but we should wait for a more compelling use case to begin actually utilizing it.

@demiankatz demiankatz closed this Mar 6, 2024
@demiankatz demiankatz removed this from the 10.0 milestone Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature small Minor changes to relatively few files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants