Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Not all fields of an entity have a label, but I still would like to see their id when they appear in the change list.
Comment | File | Size | Author |
---|---|---|---|
#25 | 3055363-24.patch | 14.38 KB | hoanns |
#22 | 3055363-22.patch | 14.37 KB | hoanns |
#20 | interdiff-3055363-13-20.txt | 2.23 KB | yogeshmpawar |
#20 | 3055363-20.patch | 5.51 KB | yogeshmpawar |
#13 | Drupal-entup_show_field_name-D8.patch | 6.01 KB | hoanns |
Comments
Comment #2
hoanns CreditAttribution: hoanns commentedDoes it test now or what is happening
Comment #3
hoanns CreditAttribution: hoanns commentedComment #4
joachim CreditAttribution: joachim as a volunteer commentedLooks good to me.
Comment #6
hoanns CreditAttribution: hoanns commentedComment #7
alexpottThanks for filing this bug report and for fixing it. Bug fixing is very valuable. However in order to commit a bug fix we need an automated to test to prove that we've fixed the bug and ensure that we don't break it again in the future. For more information about writing tests in Drupal 8 see the following links:
Comment #8
alexpottComment #9
hoanns CreditAttribution: hoanns commentedComment #10
hoanns CreditAttribution: hoanns commentedComment #11
hoanns CreditAttribution: hoanns commentedOk test added
Comment #12
hoanns CreditAttribution: hoanns commentedComment #13
hoanns CreditAttribution: hoanns commentedComment #14
borisson_I found a nitpick, that should be fixed before we can get this in. Overall this looks really good and I think when this small change is done we can mark this RTBC.
Should be
$this->t()
instead of the deprecatedt()
.Comment #15
hoanns CreditAttribution: hoanns commentedI found 0 occurences of $this->t() in the KernelTests, but 67 occurences of t(). So if this is a valid nitpick it should be adressed in a new issue. I just copied the code style from the existing class.
Comment #16
borisson_You are right! In theory this doesn't need to be
t
because it doesn't the translatability aspect of it.Comment #17
alexpottCan we change this to...
Comment #18
yogeshmpawarComment #19
hoanns CreditAttribution: hoanns commentedYou are using assertEquals instead of assertEqual, thats why you think the arguments were in the wrong order. Because these 2 methods somehow have a different order of arguments.
Comment #20
yogeshmpawarAgreed with @alexpott & comments addressed in the updated patch. Also, resolved some small nitpicks & added an interdiff.
Comment #21
hoanns CreditAttribution: hoanns commentedBut you only changed the first occurence of the bad code
Comment #22
hoanns CreditAttribution: hoanns commentedHere I removed all the critiqued occurences
Comment #23
hoanns CreditAttribution: hoanns commentedComment #25
hoanns CreditAttribution: hoanns commentedOf course I did a fucky wucky too
Comment #26
hoanns CreditAttribution: hoanns commentedComment #27
hoanns CreditAttribution: hoanns commentedComment #28
hoanns CreditAttribution: hoanns commentedWill this ever be added? it's pretty simple
Comment #29
borisson_I think the latest version of this patch looks good - the remarks made by @alexpott have been resolved, this being a bug-fix it probably still is set to the right version.
Comment #31
alexpottCommitted and pushed f66921a662 to 9.0.x and b3b6a681de to 8.9.x. Thanks!
As a bugfix going to talk to other committers about backport to 8.8.x
Comment #35
alexpott@catch +1'd the backport