Problem/Motivation
Using todays 8.x dev, I added an Entity Reference field, and on the relevant Field Settings form is Type of item to reference*
select box. The options within that select box do not have consistent character casing, as seen in the attacked image.
For example, there is Custom Block
and Custom block type
.
The only other similar problem was Unbound Display
. There could be more, but not for the entities I had available (pretty much a default standard profile installation).
Im guessing its not Entity References problem, but each entities implementation of a hook? Anyway, this is as good of a place as any to start a fix.
Steps to reproduce
Proposed resolution
Use sentence case for entity labels. See Page titles, links, and headings
Remaining tasks
Review
Commit
User interface changes
Yes.
Before
After
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#47 | interdiff-47.txt | 612 bytes | xjm |
#47 | entity-label-1987504-47.patch | 3.68 KB | xjm |
| |||
#46 | interdiff-entity-labels.txt | 1.61 KB | xjm |
#46 | entity-label-1987504-46.patch | 3.68 KB | xjm |
#43 | 1987504-34.patch | 3.54 KB | quietone |
Comments
Comment #1
rszrama CreditAttribution: rszrama commentedI'd add that the Entity Reference field name itself is also inconsistently cased when compared with other multi-word field names. I began a patch to update all instances of "Entity Reference" to "Entity reference" with exceptions for when the string referenced the module name, but then it's not clear in some parts of the module's Views integration where it might be doing so. Custom Block may be a little more straightforward.
Comment #2
amateescu CreditAttribution: amateescu commentedCan you please post the patch so everyone can chime in about the parts where you're in doubt?
Comment #3
rszrama CreditAttribution: rszrama commentedOh, I started but then trashed it when I realized I was in over my head. ; )
Comment #4
mandarmbhagwat78 CreditAttribution: mandarmbhagwat78 commentedThere are two ways to address this issue:
1. Adding a class to the $form element in entity_reference_field_settings_form() defined in entity_reference.module
add below css change into style.css
.target_type_capitalize{text-transform:capitalize;}
2. Using ucwords(strtolower($entity_info['label']))
entity_reference.module line #143
@neRok let me know if either of above solution help
Comment #5
neRok CreditAttribution: neRok commented@mandarmbhagwat78, such a 'fix' is only fixing the problem for this select box. I think it should be fixed 'deeper', ie whichever module is creating the entity and providing the name, should do so consistently (at least with core modules). This way the name is consistent across all areas of Drupal.
Or, whichever core module is implementing the hook to 'call' the entities should modify the received name to ensure it is consistent.
I think ucfirst is the correct function (most names are ucfirst style , only some are ucword style)
Comment #6
mandarmbhagwat78 CreditAttribution: mandarmbhagwat78 commented@neRok yes you are right. ucfirst() option is the correct solution. If user want to have ucword() style(that will be considered as project specific requirement), he/she can change it by applying additional class using hook_form_alter() as mentioned in my first option. I will test this out and commit the patch.
Comment #7
mandarmbhagwat78 CreditAttribution: mandarmbhagwat78 commentedHere is the patch.
Comment #8
rszrama CreditAttribution: rszrama commentedYeah, I don't think ucfirst() is worth pursuing here. Really, these names should be fixed in the various plugins that are using Camel Casing instead of Sentence capitalization. Why cover up the problem when you can fix it? : )
Comment #9
amateescu CreditAttribution: amateescu commentedThat's right, the label property from entity info annotations have to be fixed, not Entity reference.
Comment #10
neRok CreditAttribution: neRok commentedWere new issues opened to pursue each label fix?
Comment #11
mandarmbhagwat78 CreditAttribution: mandarmbhagwat78 commented@amateescu, I think there should be uniformity at the core while rendering the content.
Comment #12
amateescu CreditAttribution: amateescu commentedOk, then lets keep this issue to fix them.
Comment #21
quietone CreditAttribution: quietone as a volunteer commentedI tested this on Drupal 9.3.x, standard install. Only one of the labels is not sentence case, 'Text Editor'. That was introduced in 2204129#2 and I didn't find anything in that issue questioning the case.
The attach patch changes the label and and label_collection to sentence case.
Comment #22
Spokje- Labels changed now match other cAsInGs
If TestBot returns green, RTBC for me.
Comment #23
SpokjeRetesting (the already green) patch to make it the default for 2-daily retesting.
Comment #24
larowlanThanks for the work here folks, but I think we need a tests here to assert the case so that we prevent new instances creeping in
Comment #25
mohit_aghera CreditAttribution: mohit_aghera at QED42 commentedTrying to add a test case to ensure that titles are in proper casing.
Test only patch is failing on local.
Also, this seems bit hacky testing approach to me so I would need some suggestions whether it is fine or not.
Comment #26
mohit_aghera CreditAttribution: mohit_aghera at QED42 commentedFixing code quality check failures.
Comment #28
mohit_aghera CreditAttribution: mohit_aghera at QED42 commentedComment #29
larowlanWe're testing the UI here, but equally could we just create a kernel test that called ->getDefinitions on the entity-type manager and looped over checking the case?
Comment #30
mohit_aghera CreditAttribution: mohit_aghera at QED42 commentedComment #31
mohit_aghera CreditAttribution: mohit_aghera at QED42 commented- Updating test case to use Kernel tests.
- Refactor test case to use Annotations Discovery approach and validate strings.
- Fix one more occurrence found while running the test cases on local
Comment #33
larowlan💪 tests++
this is neat, nice work
Any reason to extend from DiscoveryTestBase, it has a test method in it that expects $this->discovery to be set (And is why it's failing)
Comment #34
mohit_aghera CreditAttribution: mohit_aghera at QED42 commentedThanks, @larowlan.
Updating test case to use KernelTests.
Comment #35
BerdirIs this really something that we should enforce/test generically?
For modules, we have the opposite rule, we explicitly uppercase each word there. It's "Automated Cron". and "Configuration Translation".
Not a native speaker, honest question: What's the reason why these _must_ be lowercased? Not saying that the examples that are updated here are wrong, just about whether we should really enforce it like this?
I suppose it's just core and we could always add an exception to the rule, still, but kinda surprised that we go with tests here :)
Comment #36
mohit_aghera CreditAttribution: mohit_aghera at QED42 commentedBased on the screenshots mentioned in #21, it seems that entity labels are following specific casing structure. I think that's why we've thought of changing labels.
Still, @quietone or @larowlan can confirm once.
Comment #37
quietone CreditAttribution: quietone as a volunteer commentedI searched the User interface standards for direction and found a section on Capitalization. The closest fit for this is the paragraph on Page titles, links, and headings, which says to use sentence case, which this patch is doing.
As berdir said in #35 modules use a different casing. The Module and theme names section does state that they are to use title case capitalization.
I think the change here is fine.
Comment #38
BerdirTo be clear, I was just surprised about the test requirement for this, I'm not aware that we do the same for modules for example. Or do we?
Just wasn't sure if this rule will really hold up all the time, but as said, it's just testing core and if we want to we could add an exception.
Comment #39
larowlanI asked for a test because I didn't want to do this piecemeal, and for it to slip with any new entity types
The test caught one we missed, so that feels like it was worthwhile
Comment #42
borisson_Patch still applies and creates (and enforces) more consistency that sounds like a good idea.
Comment #43
quietone CreditAttribution: quietone at PreviousNext commentedThis is not testing the patch. I am going to re-upload it.
Comment #45
borisson_that looks like a random failure, back to rtbc.
Comment #46
xjmThis needs to target 10.1.x now since it includes string changes.
What if the entity type label is something like "Reference URL"?
I'm okay with adding the test (as mentioned, it caught an instance we missed), but there's a risk that we might have to add special cases to it in the future for specific core entity types.
There were some grammatical issues with the code comments, so I fixed those as well as adding a comment about the above. I think it'd still be okay for me to commit this since those are trivial changes.
Comment #47
xjmGood thing I didn't just change that on commit. ;)
Comment #48
xjmCommitted and pushed to 10.1.x. Thanks!