Replacing t(' with $this->t (Group module [d8])

CommentFileSizeAuthor
#9 group-2826874-9.patch7.74 KBLOBsTerr
#7 after.png78.19 KBlomasr
#6 group-2826874-6.patch7.77 KBkristiaanvandeneynde
#2 2826874-2.patch18.02 KBlomasr
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lomasr created an issue. See original summary.

lomasr’s picture

lomasr CreditAttribution: lomasr at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commented
FileSize
2826874-2.patch18.02 KB

Adding a patch .Please review.

lomasr’s picture

lomasr CreditAttribution: lomasr at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: 2826874-2.patch, failed testing.

lomasr’s picture

lomasr CreditAttribution: lomasr at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commented
Version: 8.x-1.x-dev » 8.x-1.0-beta4
kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented
Status: Needs work » Needs review
FileSize
group-2826874-6.patch7.77 KB
  • GroupNodeDeriver does not implement StringTranslationTrait so cannot use $this->t().
  • Same for all of the cache contexts
  • Same for all of the entity classes (Group, GroupContent, etc.)
  • On top of that, Group::baseFieldDefinitions(), GroupContent::baseFieldDefinitions(), etc. are static so they can't even use $this

GroupTypeListBuilder, GroupRoleDeleteForm, GroupRoleForm, GroupPermissionsRoleSpecificForm and GroupContentViewsData did have the trait (all forms do) and could thus use it.

Attached is a smaller patch fixing the places that were valid. If we want to use $this->t() in other places, we need to look into using the trait there and whether it is useful to actually do so.

lomasr’s picture

lomasr CreditAttribution: lomasr at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commented
FileSize
after.png78.19 KB

Thanks for the clarification. I applied the patch in #6 It didn't worked for me . Please see the after.png.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

Yeah it needs a reroll.

LOBsTerr’s picture

LOBsTerr CreditAttribution: LOBsTerr at European Commission and European Union Institutions, Agencies and Bodies commented
FileSize
group-2826874-9.patch7.74 KB

I've added a reroll

i-trokhanenko’s picture

i-trokhanenko
Location Lutsk 🇺🇦
CreditAttribution: i-trokhanenko at RockSolid PSF, Drupal Ukraine Community commented
Version: 8.x-1.0-beta4 » 8.x-1.x-dev
Status: Needs review » Reviewed & tested by the community

Patch #9 applied correctly to 8.x-1.x-dev. Changes looks good for me.
+1 RTBC

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Factorial GmbH commented
Status: Reviewed & tested by the community » Fixed

Looks good, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

lomasr’s picture

lomasr CreditAttribution: lomasr as a volunteer and at Axelerant for Axelerant commented