Problem/Motivation
NodeViewBuilder hardcodes the display of the langcode field ignoring any formatter configuration. This is a barrier if you want to create new formatter such as:
- Display name with native names
- Add flag icons for languages
- Display source language
Proposed resolution
Remove the hardcode langcode element in NodeViewBuilder and keep the wrapper for BC.
Remaining tasks
Review
User interface changes
The display of the language field will change a little, because the 'field-language-display' wrapper will be gone.
API changes
The 'field-language-display' wrapper will be gone. However, this fixes a potential issue with having multiple same ID's on the same page, e.g. on a node list.
Data model changes
None
Release notes snippet
Through the removal of the hardcoded display of the langcode field, are <a href="https://www.drupal.org/node/3325438">developers be able to provide new formatters</a>, so that sitebuilder can configure a different output.
-- Original report --
Any options that are selected for language field formatter are ignored and the language field is always displayed as plain text language name.
Steps to reproduce:
- Enable translations.
- Create translatable content type.
- Select language field to be displayed.
- Check formatter option "Link to the Content".
- Create new node.
- View the node and you'll see plain text language name instead of a link.
I have noticed this when I was writing custom field formatter for the language field. It's easy to see the problem once you try to change the output for the core's language field. For example try this formatter:
namespace Drupal\custom_formatter\Plugin\Field\FieldFormatter;
use Drupal\Core\Field\FieldItemInterface;
use Drupal\Core\Field\Plugin\Field\FieldFormatter\LanguageFormatter as BaseFormatter;
/**
* Plugin implementation of the 'custom_formatter' formatter.
*
* @FieldFormatter(
* id = "language_custom_fomatter",
* label = @Translation("Language with flag"),
* field_types = {
* "language"
* }
* )
*/
class LanguageCustomFormatter extends BaseFormatter {
/**
* {@inheritdoc}
*/
protected function viewValue(FieldItemInterface $item) {
$view = parent::viewValue($item);
$view['#plain_text'] .= ' Test';
return $view;
}
}
The formatter itself is registered correctly, you can enable it for the language field. The settings form works fine, but the actual output of the formatter is totally ignored. For example instead of seeing "English Test" I always get "English".
Comment | File | Size | Author |
---|---|---|---|
#85 | interdiff_83_85.txt | 2.58 KB | jidrone |
#85 | 2637808-85.patch | 29.4 KB | jidrone |
| |||
#83 | interdiff_82_83.txt | 841 bytes | jidrone |
#83 | 2637808-83.patch | 29.43 KB | jidrone |
#82 | interdiff_72_82.txt | 24.49 KB | jidrone |
Issue fork drupal-2637808
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
SiliconMind CreditAttribution: SiliconMind at Globalbility for Globalbility commentedComment #3
Gábor HojtsyDon't know why would this happen. I think we do have plenty tests around the language formatter. Hm. Maybe we only test it in views.
Comment #4
swentel CreditAttribution: swentel as a volunteer commentedWow, this is priceless, the NodeViewBuilder class still has a check on the 'langcode' component, overriding whatever is set in any formatter for this component ... :)
Comment #5
Gábor HojtsyHahaha. Definitely needs tests.
Comment #6
swentel CreditAttribution: swentel as a volunteer commentedChanging the title
Comment #7
swentel CreditAttribution: swentel as a volunteer commentedComment #9
swentel CreditAttribution: swentel as a volunteer commentedFunny, so there's a test that sets the field on the manage display screen. And it fails without the code in NodeViewBuilder because the asserts are checking on the 'field-language-display' ID.
Now, it's actually impossible to fix this without breaking markup. The original code has a wrapper which wraps the label and the value. A formatters returns the value, but can't set a prefix for the whole field. The label will have different wrappers too.
I honestly don't think a lot of sites display this element anyway, and if they want to alter it, it's kind of tedious to do. Also, additional formatters simply don't run or change the behavior of what's on the page. I think, IMHO, we can live with removing the field-language-display wrapper. But that's not my call of course :)
Patch attached fixes the tests.
Comment #10
swentel CreditAttribution: swentel as a volunteer commentedComment #11
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedHere's an explicit test for the fact that the 'Link to content' settings doesn't work :)
Comment #13
tstoecklerCould we do something like this for BC (and remove in D9) in NodeViewBuilder?:
That would still retain the flexibility of choosing another formatter, etc.
Not sure if it would a dealbreaker to lose the wrapper either, just thinking out loud here.
Comment #14
swentel CreditAttribution: swentel as a volunteer commentedRegarding #13, yeah, that would work indeed. We could even add this as a setting in the formatter and default to TRUE ?
Comment #15
Gábor Hojtsy@tstoeckler/@swentel: depending on the formatter used, are we sure the prefix/suffix would work like that? It sounds overkill to add that to the formatter settings, but maybe an "Add field-language-display class for backwards compatibility" or some other obscure settings just to be strictly backwards compatible could work. Would be good to get some committer feedback as to how strong they feel about the need to keep that.
Comment #16
swentel CreditAttribution: swentel as a volunteer commentedTalked to alex about it - tldr - we need an upgrade path to remove the 'link to content' in case someone saved it. And a way to keep the wrapper for BC. I think the formatter setting would work fine here.
Comment #17
Gábor HojtsyLooking at LanguageFormatter::viewValue() it just inherits entity linking from StringFormatter, its parent. Maybe it was a bad idea to inherit from that... A more unique setting for language formatters is whether to display with native name.
Comment #18
johnvFor the languagefield formatter, I copied the setting from Core language, and indeed, subclassing String introduces a content link. Which is a 'feature' for my use case. However, is this also the case for core language? It will always point to itself.
Perhaps when building Views showing all translations....
Comment #19
Gábor HojtsyRight, if you are creating view with the languages as links to the translations, eg. a custom language selector block on an entity page with only the available translations listed.
Comment #21
Gábor HojtsyRemoving from sprint so it reflects what is currently being worked on.
Comment #23
xjmThe core committers and entity system maintainers agreed this issue can be handled as normal priority. (Criteria for major bugs: https://www.drupal.org/core/issue-priority#major-bug). Thanks everyone!
Comment #25
matsbla CreditAttribution: matsbla commentedI also think it would be great to have a formatter that simply display the source language of the content!
What is left to do on this issue? I tried update issue summary.
Comment #26
andrey.troeglazov CreditAttribution: andrey.troeglazov at DrupalJedi for Globalbility commentedHi all,
I`ve rerolled the patch for 8.4 branch and fixed some coding standard issues.
Comment #27
tstoecklerThis should be a lowercase "d" in "drupal".
Comment #28
andrey.troeglazov CreditAttribution: andrey.troeglazov at DrupalJedi for Globalbility commentedThank you @tstoeckler, my bad.
Comment #30
matsbla CreditAttribution: matsbla commentedThe patch in #28 is working for me!
When I apply this patch finally this module is working;
https://www.drupal.org/project/flags
I also try make a another project for different language display formatters, but it is also not working without this patch;
https://www.drupal.org/sandbox/matsbla/2901140
Is there anyway to work around this bug?
To me it seems like there is no work-around on this issue so I put back to major according to "Priority levels of issues" (Block contributed projects with no workaround.)
Comment #32
volegerComment #33
yogeshmpawarComment #34
yogeshmpawarRe-rolled the #28 patch against 8.6.x branch.
Comment #36
sealionking CreditAttribution: sealionking commentedhow about a patch for 8.5?
Comment #40
rp7 CreditAttribution: rp7 for Government of Flanders commentedRe-rolled the #34 patch against the 9.1.x branch.
Comment #42
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedFixed test case failure issue in #40, please review.
Comment #45
rp7 CreditAttribution: rp7 for Government of Flanders commentedRe-roll against 9.3.x
Comment #46
rp7 CreditAttribution: rp7 for Government of Flanders commentedMade a small mistake in the previous patch. Another attempt.
Comment #47
BerdirThis does make sense, BC as always is tricky. Render array changes are allowed. But what about template placeholders/customizations?
We should create a change record at least to explain the structure change.
Comment #51
jidrone CreditAttribution: jidrone as a volunteer and at Agileana commentedSteps to reproduce are clear so I performed to following manual test:
I guess just removing the hardcoded part is not enough on Drupal 10 versions, I didn't have the time to troubleshoot.
Would be nice to have it resolved, but based in the infrequent participation on this issue I would say this not a blocker issue for most of Drupal users so I'm changing the priority to normal.
Comment #52
Anchal_gupta CreditAttribution: Anchal_gupta at Srijan | A Material+ Company for Drupal India Association commentedRerolled Patch against 10.1x.
Comment #53
Anchal_gupta CreditAttribution: Anchal_gupta at Srijan | A Material+ Company for Drupal India Association commentedComment #55
jidrone CreditAttribution: jidrone as a volunteer and at Agileana commentedI fixed the test.
Previously, I was testing with drupalpod and for some reason it was not working for D10, but after doing a manual test with a local environment I can confirm this patch also works for that version.
Comment #56
jidrone CreditAttribution: jidrone as a volunteer and at Agileana commentedSorry, last patch had some issues.
Comment #57
andypostI think it needs upgrade path for existing sites
Comment #58
jidrone CreditAttribution: jidrone as a volunteer and at Agileana commentedI would say in this case a possible upgrade path could be to set existing language form display components to show the label and language as plain text.
What do you think?
Comment #59
andypostYes, so existing node displays will continue to work and new ones will get the same defaults as before
Comment #60
jidrone CreditAttribution: jidrone as a volunteer and at Agileana commentedAdded hook update on language module.
It only iterates through content types because the only entity type affected by this patch is node.
Comment #61
tobiasbThe inline comments (get ..., iterate) are a little bit to verbose. The update hook comment is enough for this function.
Just my 2cents. ;-)
Comment #62
jidrone CreditAttribution: jidrone as a volunteer and at Agileana commentedUploading test only patch.
Comment #64
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedRed/green as expected, @jidrone just a tip for next time if you upload the test only patch first it won't push the issue back to NW like it did here.
Comment #65
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedMinor review changes:
We shouldn't wrap test strings in t()
We don't need the second parameter here.
Also not sure if we need an update test?
Comment #66
Anchal_gupta CreditAttribution: Anchal_gupta at Srijan | A Material+ Company for Drupal India Association commentedI have uploaded the patch
Addressed #65 both points.
Comment #67
Anchal_gupta CreditAttribution: Anchal_gupta at Srijan | A Material+ Company for Drupal India Association commentedComment #68
ameymudras CreditAttribution: ameymudras at Salsa Digital commentedFixing custom command failure and some comment typos
Comment #69
smustgrave CreditAttribution: smustgrave at Mobomo commented@Anchal_gupta you can test your patch before uploading using the core scripts.
Looks good to me and the points in #65 appear to be addressed.
Comment #70
larowlanThis needs to be a post update hook, we can't rely on the entity API to be available/error proof in an update hook.
Let's use the config entity updater here
Note to other reviewers, this is correct, in HEAD even if you set it to hidden the title still displays
Comment #71
jidrone CreditAttribution: jidrone as a volunteer and at Agileana commentedChanged hook update to post update as suggested, and also using config entity updater.
Thanks @larowlan.
Comment #72
jidrone CreditAttribution: jidrone as a volunteer and at Agileana commentedSorry, last patch had some paths wrong.
Comment #73
smustgrave CreditAttribution: smustgrave at Mobomo commentedTests are all green
Patch applies cleanly
Running tests without the fix fail as expected
Update runs without issue
Code looks clean to me.
But will still need a change record
Comment #74
larowlanCan we get a change record/release note here? Fine to self RTBC after adding
Comment #75
smustgrave CreditAttribution: smustgrave at Mobomo commentedTook a shot at the change record. Mostly borrowed from the issue summary since the IS was so well done.
Not sure what's needed for a release note.
Comment #76
jidrone CreditAttribution: jidrone as a volunteer and at Agileana commentedI did some changes to the change record to make it more clear.
Comment #77
smustgrave CreditAttribution: smustgrave at Mobomo commentedThanks! So can this go back to RTBC
Comment #78
xjmWe still need a release note, though. (Ideally, the release note will link the change record.)
Comment #79
tobiasbDone.
Comment #80
andypostit needs upgrade path test
Comment #81
jidrone CreditAttribution: jidrone as a volunteer and at Agileana commentedAdded test for the post_update.
Comment #82
jidrone CreditAttribution: jidrone as a volunteer and at Agileana commentedGit was recognizing one file as binary when it generates the patch, but I found a workaround.
Comment #83
jidrone CreditAttribution: jidrone as a volunteer and at Agileana commentedFixed CS error.
Comment #84
jidrone CreditAttribution: jidrone as a volunteer and at Agileana commentedThose dump files for update tests produce a lot of weird errors, hopefully I fixed all.
Comment #85
jidrone CreditAttribution: jidrone as a volunteer and at Agileana commentedMaybe disabling the Spell check on some config imports.
Interdiff agains 83 because 84 didn't improve the issue.
Comment #86
smustgrave CreditAttribution: smustgrave at Mobomo commented+1 for me but will let someone else see what they think.
Comment #87
rpayanmI tested manually the patch from #85.
Test #1
1. I used an empty Drupal 10.1.x
2. Applied the patch
3. Installed Drupal
4. Followed the patch from the IS
It worked as expected.
Test #2:
1. I used an empty Drupal 10.1.x
2. Installed Drupal
3. Followed the steps from IS
4. Applied the patch from the IS
5. Ran "drush updb".
It worked as expected, but I had to repeat the step 4: Check formatter option "Link to the Content", because it was unchecked after the update.
Let me know if I did something wrong.
Comment #88
jidrone CreditAttribution: jidrone as a volunteer and at Agileana commentedHi rpayanm,
That's the expected behavior after running the update, because it sets the formatter to match the hardcoded values to avoid unintended results on existing sites using that formatter.
Comment #89
rpayanmOh ok, thank you.
Comment #90
catch#2226493: Apply formatters and widgets to Node base fields says it covered this, but looks like it didn't?
The logic here also needs to run in config entity presave so that it's applied to configuration shipped with modules and install profiles. Quite a lot of examples of this in 9.5 and 10.1, especially but not exclusively in views.
Does the configured formatter end up in the same place in the render array?