Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
When user login, "Member for" label is invisible
Possible solution:
Comment | File | Size | Author |
---|---|---|---|
#61 | 2638250-61.patch | 6.58 KB | kksandr |
#58 | interdiff-2638250-50-57.txt | 5.58 KB | mohit_aghera |
#58 | 2638250-57.patch | 9.59 KB | mohit_aghera |
#50 | 2638250-50.patch | 5.03 KB | mohit_aghera |
#35 | member.for-2638250-35.patch | 2.74 KB | geerlingguy |
Comments
Comment #2
VladimirAusComment #3
markhalliwellI don't think it should be the Bootstrap "label" (class) component at all.
It should probably be an actualOr possibly just<label>
DOM element?<strong>
?Comment #4
markhalliwellActually, this is a core issue. The problem is that user_user_view() is hard coding this "label" class instead of using a proper render array that allows themes like us to tap into it.
Core does have a "label" render element type, it uses the form_element_label theme hook. I'm not entirely sure if that would be appropriate, but it may be considering that it's already using the "item" element type (which is why it's being wrapped by form_element).
The class "label" is used by bootstrap to denote a label component, which is not what we want here. Even if we did, the point is that we couldn't modify it to add the secondary (necessary)
label-default
class to make it show up (as in the screenshots above).Comment #5
markhalliwellAt second glance (and considering that it's already using the "item" element type), I think just moving it to the #title property should fix the problem. Let me see if I can't roll a patch quickly.
Comment #6
markhalliwellComment #7
heykarthikwithu@markcarver
On user login, i am able to see the "Member for" label (label is not hidden)
Are their any other ways to reproduce the bug?
Comment #8
markhalliwell@heykarthikwithu, use the Bootstrap base theme. However, that isn't what this issue is about. The label not showing up (because it's white text on white background) is just a side affect of this markup being hardcoded.
Comment #10
hkirsman CreditAttribution: hkirsman commentedOn my site the fields are in
<div class="field__label">Label here</div> so after applying the patch there's no formatting.
Also what bugs me is that there's no option in the admin to set the label to above, inline or hidden.
Comment #12
mlncn CreditAttribution: mlncn at Agaric commentedAgree with needing to make the "Member for" output easily overridable with a template, but this changes markup from what is currently output— and doesn't actually provide any template suggestions to make overriding easy.
Comment #15
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedRe-roll the last patch. There are still outstanding questions, but it definitely helps solve the problem "'Member for' is displayed differently from other user fields".
@mlncn
True , but I would say that's because the current output is wrong. Without this patch "Member for" is quite different from any other fields that you add using /admin/config/people/accounts/fields - it is h4 and no other field is. After this patch, they are almost identical - the only thing missing is a colon after "Member for".
Good point.
@hkirsman
That's a limitation of
hook_entity_extra_field_info
which is what is being used here.Arguably a better solution would be to expose the 'created' base field directly, which is closer to what happens on a node. However I'm not sure how feasible it is to change this now - could be tricky to avoid back-compatibility problems for existing sites??
@markcarver
The class "label" seems to be used in several places by Drupal core. I agree it's unfortunate that the bootstrap framework uses the same class for a different reason and I have raised #2923978: Drupal labels disappear for that. However that seems to be a problem for one specific contrib theme, and it shouldn't stop us from using the label class if that is the right thing for Drupal core.
Comment #16
markhalliwellThis adds a theme hook suggestion so it can be targeted.
---
That being said, this "field" should really be converted into an actual field, not some arbitrary afterthought injected in an alter hook.
Comment #17
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedThanks @markcarver, that seems like the right fix here - it addresses the specific problems in the title, and is definitely a step in the right direction.
If anyone feels inclined they could raise a follow-on issue for "use an actual field".
Comment #18
xjmHm, not sure about this change. It does change the markup and the appearance, including the layout of the element so that it's now on two lines instead of a single line. It's also switching the field from a semantic element in HEAD (
<h4>
) to<label>
, which I don't think is supposed to be used for non-form elements.Tagging for accessibility review to double-check that.
Comment #19
AdamPS CreditAttribution: AdamPS at AlbanyWeb commented@xjm thanks for your review.
Do you think it would be better to fix this 'properly'? Now that we have #2392845: Add a trait to standardize handling of computed item lists maybe we could swap 'member_for' from
hook_entity_extra_field_info
intobaseFieldDefinitions
withsetComputed(TRUE)
? Then it definitely would be formatted exactly like all other fields. The core installation profiles could set'label' => 'inline'
to make it on one line. It would be a more complex fix, not sure if anyone here would have time to do it.Comment #20
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedHere's an alternative approach switching to a computed field. It seems much cleaner and the output is identical to other fields of course. It also allows the site builder to customise the format.
The patch likely needs more work to fix tests and provide a hook_update. However I'd like to get some feedback on the general concept first.
Warning: with this patch you will see this issue #2686409: Time Ago summary does not render on Manage Display for Timestamp and Datetime fields
Comment #22
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedComment #23
markhalliwellIf this is the case, then I would imagine that all
item
render elements (\Drupal\Core\Render\Element\Item
) should be using a different#theme_wrapper
thanform_element
. Regardless, that would be out of scope for this issue and is really an entirely separate issue from this one.That being said, I really do prefer the computed field approach since it doesn't deal with a custom render array at all and allows an actual field to be rendered/targeted. Plus converting this into a "proper" field means that it doesn't use
<label>
and users can choose where said label is displayed, so "win-win-win".---
The only setting that needs to be here is
past_format
. The rest can be removed since they are the same as\Drupal\Core\Field\Plugin\Field\FieldFormatter\TimestampAgoFormatter::defaultSettings
.I think the weight should be the same as the original: 5.
Comment #24
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedComments done thanks.
I would be very grateful if someone can explain why my patches for the new file are failing to apply. I ran git diff as normal. The error output is
error: dev/null: No such file or directory
Comment #26
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedNever mind it seems to have applied this time.
Comment #27
idebr CreditAttribution: idebr at iO commentedClosed #2698019: user "member for x time" is hardcoded as a duplicate issue.
Comment #28
markhalliwellThe failing tests in #24 is because this new (proper)
member_for
field needs to be added to the expected output.We should also probably add a test to ensure that this computed field outputs as expected? At the very least so we don't delete it or change it to a manual render array again. Idk.
Comment #29
AdamPS CreditAttribution: AdamPS at AlbanyWeb commented@markcarver Thanks for the analysis. I am not familiar with JSON, so please bear with me asking a basic question. Would live sites also see the extra field appearing in JSON output? Is this definitely desirable? - or is it potentially a back-compatibility problem?
Comment #30
markhalliwellYes and probably, yes.
No, because this is adding a new field. It would only be a BC problem if existing sites relied on a field that was suddenly taken away.
Comment #31
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedThanks for explaining. I'm out of time for now, so if anyone else wants to fix the tests please go ahead.
Comment #34
geerlingguy CreditAttribution: geerlingguy at Acquia commentedJust ran into this for the nth time, and noting that the whole feature seems a bit strange to me; besides a certain subset of site types, the value of having "member for" as a displayed-by-default field on the user profile doesn't seem desirable. I've used CSS to hide it way too many times... but maybe for Drupal 9 we can just drop it, and if someone wants to replicate the functionality, they can either add it via theming, or by a computed field or something?
Comment #35
geerlingguy CreditAttribution: geerlingguy at Acquia commentedAttaching re-roll against 8.7.x (no diff, patch from #24 re-applied cleanly) for the testbot to take another stab.
And I think we would definitely need to add a test for this field (to prevent regressions), and either:
In either case, this would be a frontend change which could cause site owners to get a little upset (their theming of the user profile would most likely be affected, especially if they hid the 'Member for' output before, and it popped back in as a new field), so it would need a change record and be in a new minor release (at least).
Comment #37
geerlingguy CreditAttribution: geerlingguy at Acquia commentedWhen I visit the 'Manage display' page for user accounts, I get the following errors with the above patch:
Comment #40
Christopher Riley CreditAttribution: Christopher Riley commentedHas this issue ever been resolved? It seems trivial I know but this has been goobered long enough and should be a simple fix.
Comment #43
djg_tram CreditAttribution: djg_tram commentedOnly five years and still the same bug. :-))
Comment #46
larowlanI agree the computed field is the best approach
Comment #47
BerdirNot convinced, an extra field definition has non-trivial overhead, seems quite complicated. The _only_ thing the computed field now does is display created with a different label. Otherwise it's just duplicating the created field.
We could experiment with just rendering the created field with the timestamp_ago formatter which we've had since 2014 for the extra field.
A feature to override the label for display would be useful, because then we could possibly just drop this entirely and if you want it you could show the created field with that label (or any other). There are many use cases for that.
Comment #48
larowlanThat sounds much better
You should be able to use a base field override to change the label, so in theory that could go in the standard profile because this feature doesn't belong anywhere but there in my book
Comment #50
mohit_aghera CreditAttribution: mohit_aghera at PreviousNext commented- Updating the initial patch with the approach discussed in #47 and #48
- I haven't included the interdiff as it is completely different approach compared to previous patches.
- Doing one test bot run manually to see possible failures.
Comment #51
Medha KumariReroll the patch #16 with Drupal 9.5.x
Comment #52
Medha KumariReroll the patch #16 with Drupal 9.5.x
Comment #53
Medha KumariReroll the patch #16 with Drupal 9.2.x
Comment #54
Medha KumariReroll the patch #16 with Drupal 9.2.x
Comment #55
prasanth_kp CreditAttribution: prasanth_kp as a volunteer and at Zyxware Technologies commentedPatch #52 failed to apply on Drupal 9.2.x-dev.
Comment #56
mohit_aghera CreditAttribution: mohit_aghera at PreviousNext commentedI think based on feedback on #47and #48, the implementation approach in #50seems correct.
I am hiding other patches from #51 to #54
Comment #57
mohit_aghera CreditAttribution: mohit_aghera at PreviousNext commentedComment #58
mohit_aghera CreditAttribution: mohit_aghera at PreviousNext commented- Fixing multiple test cases and adding a configuration files on user module's config/install folder.
- Uploading and triggering test bot to see if adding file in module's config/install folder causes more failures.
- There are still a couple of test cases related cache are failing. Debugging those.
Keeping it assigned to me
Comment #59
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.
Tagging for an issue summary update as the current one is outdated and there is no proposed solution that matches the patch. Should happen probably first so the agreed upon solution is seen by everyone
Testing on an Umani install for 10.1.x and the label "Member for" appears
But the feature of being able to change the label as proposed in #47 I don't see.
At this time we will need a 10.1.x patch as #58 does not apply to 10.1.x
Please do not just reroll before understanding the agreed upon solution in the IS.
Comment #61
kksandr CreditAttribution: kksandr commentedReroll the patch #57 with Drupal 11.x