Regression: More Labels No Longer Translated

Configuration settings should not be passed through the t() function, so I believe reverting this change would be incorrect.

The correct way to translate configuration is with Configuration Translation, which relies on the schema. I see that currently in the schema, the more link text is defined as a "string". To make it translatable, this would need to be changed to a "label".

I agree a test for this would be desirable. I haven't written a test involving translation, so it might be a good learning experience.

Okay, I had a look at enabling config translation.
Changing the schema is easy.
For some modules this would automatically enable config translation, but for others a .config_translation.yml file. However, I haven't been able to figure out what to put in this file for a field formatter.
Any advice appreciated.

Digging into this a bit to find the proper way to do this...

• There are no instances of the following in Drupal core: t($this->getSetting(', so we probably want to avoid doing it this way (the way we were doing it, which is why it was removed in the first place).
• A cursory search of some commonly-used contrib modules did not turn up any occurrences of this pattern either.

With the recent commits to this module, I do think it is safe to make the following change in src/Plugin/Field/FieldFormatter/SmartTrimFormatter.php. From

$more = $more_settings['text'];

To

$more = $this->t($more_settings['text']);

Granted, this is similar to what we were doing in 2.0.0, but after looking at this for a bit, I'm not sure there is any way for us to get around the phpcs message "Only string literals should be passed to t() where possible" on code like this. There are multiple occurrences of this type of pattern in Drupal core.

Test added as well. MR coming momentarily.

-mike

MR created. Tests are passing with a phpcs warning about the "Only string literals should be passed to t() where possible" thing - I really don't think there is a way of getting around it seeing how the user can set the "More text" string. I don't feel all that bad about it though, as both core and contributed modules do this sort of thing (and have the same phpcs warning).

-mike

I'm on vacation and can't look into this more at the moment, but I think @lostcarpark was on the right track in that we need to add a *.config_translation.yml file with the ajax base_route_name for the field formatter.

I feel the way we should be doing it is through configuration translation. That's super easy to set up for a standard configuration form. But the field formatter settings are a bit more challenging because it's embedded in the settings for the entity field configuration, so we'd need to figure out how to set the base_route_name to allow translation for all entity types. I'll have another look, and see if I can make it work.

Setting back to Needs Work for now. This MR is good, but lets let @lostcarpark take a stab at the config. Also there is a PHPCS error on the _smart_trim_test_translation() function name that probably should be addressed.

I have done some digging on config translation of entity field formatters, and there is a long running issue open in Drupal core to support this in config translation. It looks like it might be moving towards a conclusion, but it's impossible to say when it might make it into core:

🐛 Entity view/form mode formatter/widget settings have no translation UI Needs review

I've tested the patch in MR !4294, and if core is patched, the only change needed in Smart Trim is updating the schema to turn relevant strings into labels.

After applying the patch, the following steps are needed to translate the More label:

1. Configure Article node display and change format for body field to Smart Trim, making sure to enable the More section (/admin/structure/types/manage/article/display/teaser)
2. Enable the Config Translation module (/admin/modules).
3. Add a language (/admin/config/regional/language).
4. Go to configuration translation page (/admin/config/regional/config-translation).
5. Click on List button for Content view display
6. Click on Translate button for Teaser view mode for Article content type
7. Click on add for the language you added.

The config translation screen should look like the following:

I've made the schema changes and submitted as MR !69. Unfortunately, I don't think it's possible to write a test for this at present, as the test would be dependent on the core patch. I have manually tested after applying the core patch, and not encountered any problems.

Personally, I think this is the correct approach to take, but it does pose a problem until the core issue is resolved. One option would be to temporarily adopt MR !66, and remove it after the core fix is completed. Personally, I would prefer to avoid this as it introduces code that goes against best practice. If users wish to use this approach, I would suggest they instead apply MR !66 as a patch.

If we adopt MR !69, it should be a more future proof solution. Users will have to decide if they want to patch their core to enable it.

I think I will respectfully partially disagree with @lostcarpark on this one.

1. I agree that !69 is the best solution in the future - once the core patch is committed and there is a new Drupal release that supports it. If/when this happens, we will have to be very clear in the release notes that the label translation functionality is only available in Drupal Core 10.something.whatever+.
2. I disagree that we shouldn't merge !66 in the meantime and ask users to apply the patch if they want the functionality. Granted, it is not the absolute best practice, but there are multiple examples in both core and contributed modules where the same pattern is used. It is a perfectly valid temporary fix until !69 has a clear path.

-mike

While I really would like to translate the correct way, in this case I don't think we should let perfection be the enemy of the good. I agree with @ultimike that we should merge in !66.

While we wait for for https://www.drupal.org/project/drupal/issues/2546212 🐛 Entity view/form mode formatter/widget settings have no translation UI Needs review to get fixed we should split !69 into a follow up task that is postponed.

!66 will also be backwards compatible. Quite selfishly, one of my client's sites will then continue to be translated after I update the module with that fix.

And it turns out contrib modules can require core patches: https://drupal.slack.com/archives/C392CHBEW/p1692642139636439 and https://github.com/cweagans/composer-patches/blob/1.x/README.md#allowing...

I agree that we shouldn't require a core patch for this fix, even if it might be possible with Composer.

So I think the best thing for the immediate term is to merge Mike's change !66.

I've reviewed the change, and the change looks good, and his test looks very neat. I just don't like what it's doing very much.

When 🐛 Entity view/form mode formatter/widget settings have no translation UI Needs review gets resolved (and released in a Drupal release - I presume it is a new feature rather than a bugfix, so it would probably be a minor release), I propose we remove the t() and implement the schema test, and rewrite the test to cover the new implementation. As this change will break backwards compatibility it should be a new major version (e.g. 3.0, or whatever version we happen to be on at the time), and it should increase the minimum Drupal version to match.

It would probably be an option to merge the schema change now, since it wouldn't have any negative impact, but it probably makes sense to hold it and merge all together.

I've created 🐛 Change to use Config Translation for More label Postponed as a follow-on issue for when 🐛 Entity view/form mode formatter/widget settings have no translation UI Needs review has been merged.

I'm not sure if it's possible to move MR !69 to that issue, or if I need to create a new MR and cherry-pick the change.

I've made some changes to MR !66 to fix PHPCS reported issues.

First in SmartTrimFormatter.php, I don't think there's any way we can use the t() function to wrap a config setting that PHPCS won't complain about, so I've added a // phpcs:ignore comment to tell it to ignore that line. Normally, I don't like doing this, as it's equivalent to covering up the smoke detecter. However, in this case there is a planned fix and this will be removed in future. I think it's better to use this than to have PHPCS failing and risk future issues being missed.

I've also added a @todo comment to link the core issue and indicate the permanent fix will be coming.

Next, the _smart_trim_add_translation function name was not compatible with PHPCS's requirement that all functions in a .module file must start with the module name (in this case, the test module name). I understand the logic if prefixing with _ to indicate a utility function, but I think the PHPCS rules have to trump that. I have renamed to smart_trim_translation_test_add_translation, which I think is still clear.

PHPCS is now passing in the pipeline.

Hope you agree with these changes. Happy to reverse them out if you don't like them.

Yep - I'm cool with all of the changes made in comment 18 - thanks, @lostcarpark!

-mike

Had to do a rebase but then merged in #66. Thanks for the help and future tickets.

Automatically closed - issue fixed for 2 weeks with no activity.