Problem/Motivation
As it has been introduced in #1850080: Entity type labels lack plurality, cannot generate UI text based on label if plural is needed now we have a way to specify plural label for any entity type.
Proposed resolution
Add plural labels to all entity types defined in core. More information can be found in the change record: https://www.drupal.org/node/2689949
Remaining tasks
- List all entity types and determine which still need labels.
- Determine the correct capitalization patterns in English and make it consistent.
User interface changes
Nope.
API changes
Nope.
Data model changes
Nope.
Comment | File | Size | Author |
---|---|---|---|
#70 | reroll-diff-2702683-64-69.txt | 844 bytes | Manuel Garcia |
#69 | 2702683-69.patch | 29.42 KB | pguillard |
#64 | 2702683-64.patch | 29.45 KB | Manuel Garcia |
#64 | interdiff-2702683-59-64.txt | 16.21 KB | Manuel Garcia |
#59 | 2702683-59.patch | 27.82 KB | Manuel Garcia |
Comments
Comment #2
lucian.cazaciuc CreditAttribution: lucian.cazaciuc commentedComment #3
lucian.cazaciuc CreditAttribution: lucian.cazaciuc for ASSIST Software SRL commentedI fixed it with the help of amateescu and the great DrupalHeroes at DrupalTransilvania Camp
Comment #4
jibranComment #5
xjmI don't think it makes sense to scope these issues on a per-module basis. Please see:
https://www.drupal.org/core/scope#incomplete
(Edit: more applicable fragment link)
Retitling to what I think the correct scope should be. Let's close the other issues as duplicated and add them to this patch.
Comment #6
xjmGuess it applies to both content and config. Does someone want to do a quick count of how many there are that should have plural labels, and add that to the summary?
Comment #7
xjmHere is the combined patch from the RTBC issues that still applied.
Search and aggregator did not.(Actually they are in the patch, must have been my mistake.) Thanks to everyone who helped on the other issues -- I'll try to list all the contributors on here shortly.Also, 8.1.x is in string freeze, so moving to 8.2.x.
Comment #8
xjmNW for a list of which entity types need fixing and have been fixed per #6.
Comment #9
xjmdpopdan, bojanz, rszrama, vasike, vdanielpop, rteijeiro, pguillard, jmikii, svjuly, Drupa1ish, jhodgdon, ciprian.stavovei, Wim Leers, Gabr1el all contributed to the other issues I closed as a duplicate so far. (Edit: updated). amateescu is also mentioned as a mentor and should also be credited.
Comment #10
xjmBTW, if anyone is wondering "Why would we bother to combine all these when they are already done?" see for example:
#2702695-4: Add plural labels to Entity Types in editor module
If we split the change up into lots of tiny patches, then a reviewer cannot ensure it is applied consistently across core.
Comment #11
vdanielpop CreditAttribution: vdanielpop at PitechPlus commentedThe purpose of the issues created separately was to introduce the new guys to the workflow, but you're right, having all the changes together makes it clearer for the reviewer. Thanks :)
Comment #12
xjmAdding in the patch from the Editor issue, and quoting the full comment:
I can actually see the case for either. Capitalization is translation-specific, but with this new API, we will allow each language to specify a (more) grammatical capitalization for that language. (It still runs into trouble if used in a sentence but that's a limitation of the original issue.) In English, I could see cases being made for both ways. We say (e.g.) "the Taxonomy Access Control module" so I think by that reasoning it would be capitalized, but I know core goes to lengths to try to lowercase things in other contexts, so let's get more feedback on that point.
Comment #13
xjm(NR to trigger the test run.)
Thanks @vdanielpop! I can see the benefit in that too. (Also, adding your issue credit from the other issue -- thanks for commenting!)
Comment #14
xjmAdding a note about making capitalization consistent to the summary. Some instances are lowercase and others are uppercase, so we need to make it consistent one way or the other in English.
Comment #15
xjmComment #16
bojanz CreditAttribution: bojanz at Centarro commented+1 from me on merging into 1 patch. I'll review tomorrow.
We definitely want the newly introduced labels to be lowercase btw.
Comment #17
pguillard CreditAttribution: pguillard commented+1 !
Comment #18
therealssj CreditAttribution: therealssj commented+1!
Also aren't the labels automatically converted to lowercase later when retrieving their value?
Comment #19
bojanz CreditAttribution: bojanz at Centarro commentedNo, they aren't.
Comment #20
rszrama CreditAttribution: rszrama at Centarro commentedJust pinging in from #2702775: Add plural labels to Entity Types in Taxonomy module; my labels are already in the patch. : )
Comment #21
bojanz CreditAttribution: bojanz at Centarro commentedWe're missing the parts from #2702797: Add plural labels to Entity Types in User module and #2702709: Add plural labels to Entity Types in Field module.
Comment #22
pguillard CreditAttribution: pguillard commentedI merged them, though they are not RTBC at the moment.
I hope I understand @bojanz request !
Comment #23
ztl8702 CreditAttribution: ztl8702 as a volunteer commentedAs a matter of style, I would like to know which one is more acceptable?
or
The difference is the comma after the last item in the array (or the last argument).
There isn't a comma here: https://www.drupal.org/coding-standards/docs#Plugin
Comment #24
ztl8702 CreditAttribution: ztl8702 as a volunteer commentedSee line 24 here .
I think in most cases we don't add a comma. But there are exceptions like line 19 here.
Comment #25
bojanz CreditAttribution: bojanz at Centarro commentedWe want to always have a trailing comma. That's in line with our regular array coding standards.
Comment #26
ztl8702 CreditAttribution: ztl8702 as a volunteer commentedOK. Fixed commas.
Comment #27
rosinegrean CreditAttribution: rosinegrean at PitechPlus commentedAs @jhodgdon commented here https://www.drupal.org/node/2703103#comment-11083865
merged in the patch for the Tour entity
Comment #28
vg3095 CreditAttribution: vg3095 commentedAdded plural labels for System, Views and Node module.
Comment #29
vg3095 CreditAttribution: vg3095 commentedBy mistake , uploaded interdiff as a patch file ,the extension should be txt, sorry for that .
Comment #30
xjm(Updating issue credit.)
With regard to the trailing commas in annotations, it's our coding standard for normal PHP arrays. However, originally, the Doctrine annotations did not support the final trailing comma. This was eventually fixed, but there are still a lot of the original style in core for that reason. https://www.drupal.org/node/1354#annotations does not specify one way or the other, so I agree it makes sense to be consistent with the PHP array coding standards in absence of that.
Thanks everyone.
Comment #31
Drupa1ish CreditAttribution: Drupa1ish at EuroDomenii commented#2703067-5: Add plural labels to Entity Types in Shortcut module, thx @xjm
Comment #32
pashupathi nath gajawada CreditAttribution: pashupathi nath gajawada as a volunteer and at Melity commentedI am looking at it.
Comment #33
pashupathi nath gajawada CreditAttribution: pashupathi nath gajawada as a volunteer and at Melity commentedComment #36
rosinegrean CreditAttribution: rosinegrean at PitechPlus commentedDuplicate of Add plural labels to entity types
Comment #37
tim.plunkettThis is marked as a duplicate of itself, did you mean #2702799: [meta] Add plural labels to core entity types ?
Comment #38
rosinegrean CreditAttribution: rosinegrean at PitechPlus commentedSorry about this.
I wanted to update all issues with DCTransylvania tag. Last year @amateescu created an issue for each entity type as a novice task during the camp and most of them were marked duplicated of this one along the way.
Since next weekend we will be holding a new sprint during the camp, i wanted to have a clean list.
Thanks again for noticing,
Comment #39
rosinegrean CreditAttribution: rosinegrean at PitechPlus commentedLet's see if there is still work to be done on this.
Comment #40
rosinegrean CreditAttribution: rosinegrean at PitechPlus commentedSo, i needed to re-roll the patch and update for Comment, CommentType and Editor.
The rest are as they were before.
@tim.plunkett do you think there is something else to do for this one ?
Thanks,
Comment #41
tstoecklerthis should be lowercase.
This is tough, because actually each entity is a set of settings already, so I think this needs more thought.
We should add the context to all new labels, as well.
Also this is not covering the new
label_collection
yet. Not sure if we should do that here or in a followup.Comment #42
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedAddressing #41
About #41.2 - for the plural of content language settings i went with 'content languages settings' - does that make sense?
Comment #44
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThe 'label_collection' issue brought up by @tstoeckler made me realize that we should probably fix/finalize the API for defining all these labels, so we don't end up with different solutions for 8.3.x vs. 8.4x, so I opened #2869039: Group all entity labels under a single 'labels' annotation key and I think we should do this one as a follow-up.
Comment #45
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedPostponing on #2813895: Combine entity type labels in an array - Though that is far from ready, there's not even a patch there yet...
Comment #47
Wim LeersComment #49
bojanz CreditAttribution: bojanz at Centarro commentedLooks like #2813895: Combine entity type labels in an array isn't happening.
#2942588: EntityListBuilder::render() should use the entity type plural label in the empty text shows that using better APIs gives us worse UI text because this issue hasn't landed in a year.
So let's proceed. Once [##2813895: Combine entity type labels in an array happens, the labels can be updated, like with any other change.
Comment #50
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedComment #51
jofitz CreditAttribution: jofitz at ComputerMinds commentedLet's start with a re-roll then see where we stand...
(I think we can safely unassign @prics after 11 months)
Comment #53
jofitz CreditAttribution: jofitz at ComputerMinds commentedAdjust tests to match new functionality.
Comment #55
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedThanks @Jo Fitzgerald!
I searched for all entities extending
ContentEntityBase
, and we were missingFile
andMenuLinkContent
, so added support for those as well. I believe now all are covered.Also worked on the failing tests. Let's see if I broke others...
Comment #56
tstoecklerThese should all be lowercase.
Missing the singular label.
Comment #57
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedThanks @tstoeckler!
Addressing #56 and fixing a failing test.
Comment #58
hchonovThere are entity types that are not yet covered in the patch - Workflow, RestResourceConfig, Block, FilterFormat, Vocabulary.
Comment #59
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedGood call @hchonov, thanks. Added labels to Workflow, RestResourceConfig, Block and FilterFormat. Vocabulary already has them.
Comment #60
tstoecklerLooks great to me, thanks!
The only remaining question is whether we want to handle collection labels (i.e. uppercase plural label) here as well. I don't feel strongly either way. On the one hand it shouldn't be that much work, on the other hand I don't want to hold this up any longer.
So I'm fine with this being RTBC directly, but also fine with needs work.
Comment #61
hchonovIf it isn't that much work, then we will not be holding up this that much longer, so let's do it here.
Comment #62
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedHappy to work on that, what exactly do you mean?
Comment #63
tstoecklerSo we should add an entry similar to the following to all entity types:
Comment #64
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedSimple enough, here it is. Also reordered the changes to
Editor
so its consistent with the rest.Comment #65
tstoecklerPerfect, thank you!
Comment #66
bojanz CreditAttribution: bojanz at Centarro commentedLooks good, +1 for RTBC.
Comment #67
Gogowitsch CreditAttribution: Gogowitsch at QuoData GmbH Quality & Statistics for ÖQUASTA commentedTested & looks good, thanks everyone! For my project I really need this in core.
Comment #69
pguillard CreditAttribution: pguillard commentedA a little reroll, one change not applying anymore on 8.6.x
I guess I should not set it back to RTBC.
Comment #70
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedThank you @pguillard, reroll looks good.
Setting back to RTBC assuming it comes back green :)
Comment #78
alexpottAdding commit credit according to #9 and comments on this issue.
Comment #79
alexpottCommitted 1382290 and pushed to 8.6.x. Thanks!
Comment #82
rosinegrean CreditAttribution: rosinegrean at PitechPlus commented