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.
Comment | File | Size | Author |
---|---|---|---|
#28 | interdiff_26-28.txt | 725 bytes | anagomes |
#28 | 3124369-28.patch | 4.96 KB | anagomes |
#26 | interdiff_2-26.txt | 1.94 KB | anagomes |
#26 | 3124369-26.patch | 4.37 KB | anagomes |
#22 | interdiff_20_21.txt | 757 bytes | sanjayk |
Comments
Comment #2
swatichouhan012 CreditAttribution: swatichouhan012 at Valuebound for Valuebound commentedKindly review patch.
Comment #3
sanjayk CreditAttribution: sanjayk as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #4
sanjayk CreditAttribution: sanjayk as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedI have reviewed above patch and it's working fine but still need to replace t() with $this->t() in some files. I have created a patch for that.
Comment #5
Berdirthis is not a class.
Comment #6
Deepak Goyal CreditAttribution: Deepak Goyal at Srijan | A Material+ Company for Drupal India Association commentedComment #7
Deepak Goyal CreditAttribution: Deepak Goyal at Srijan | A Material+ Company for Drupal India Association commentedHi, @Berdir
Updated patch please review.
Comment #8
Pooja Ganjage CreditAttribution: Pooja Ganjage commentedHi,
I am applying patch with using \Drupal\Core\StringTranslation\StringTranslationTrait class.
Please review once.
Comment #9
Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commentedComment #10
BerdirThe patch doesn't apply.
this seems to add an extra empty line with spaces.
here too.
changes to this class seem to be unecessary/out of scope, it already has the StringTranslationTrait from the parent class.
Injecting the service is optional and core isn't doing it most of the time, the trait offers a setter that could be used for unit testing if really necessary.
Comment #11
sanjayk CreditAttribution: sanjayk as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #12
Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commentedHi,
Attached updated patch.
Kindly review this patch once.
Thanks.
Comment #13
Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commentedComment #14
sanjayk CreditAttribution: sanjayk as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented@Pooja Ganjage have same issues "No such file or directory" in both #8 and #12 please fixed it.
Comment #15
Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commentedHi,
Attached updated patch.
Kindly review once.
Let me know for any correction.
Thanks.
Comment #16
Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commentedNeeds Review
Comment #17
sanjayk CreditAttribution: sanjayk as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented@Pooja still two files are not found please follow the instruction here under "More Git Commands" https://www.drupal.org/node/707484 and add files in patch.
Comment #18
sanjayk CreditAttribution: sanjayk as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedUploading correct patch.
Comment #20
sanjayk CreditAttribution: sanjayk as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedSorry by mistake uploaded wrong patch
Comment #21
sanjayk CreditAttribution: sanjayk as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented@Berdir by mistake added test for D9.1 but test cases pass for D8.
Comment #22
sanjayk CreditAttribution: sanjayk as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedPlease ignore #21 attached interdiff.
Comment #23
sanjayk CreditAttribution: sanjayk as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #24
TR CreditAttribution: TR commentedThis is getting further and further away from where it should be.
Both EntityViewsData and ConfirmFormBase already have StringTranslationTrait, so that is inherited by the Entity classes. All you have to do is use $this->t() instead of t().
The patch in #2 is essentially correct. There may be a few other places that should use $this->t(), but subsequent patches added out-of-scope changes and shadowed properties and methods by overriding the inherited properties and methods of the parent classes. You can inject string_translation or not (it's not technically wrong but there's really no reason to do it), however you shouldn't be shadowing the parent properties and methods. @Berdir already said this in #10.
Comment #25
anagomes CreditAttribution: anagomes at CI&T commentedComment #26
anagomes CreditAttribution: anagomes at CI&T commentedI'm uploading a patch based on the one in #2, with some small changes, which I believe are in the scope of this issue and solves the problem. Running phpcs, there are no warnings about this matter anyway.
Comment #28
anagomes CreditAttribution: anagomes at CI&T commentedUploading a new patch with small corrections, since the last one didn't pass the tests.
Comment #30
TR CreditAttribution: TR commentedNote that the Entity API currently has 14 failing tests - any patch you submit will be "Waiting for branch to pass" until you click through and select "Don't wait". I have done that now to get your patch tested.
Best case, you should have 14 failing tests.
The patch looks pretty good to me. Let's see what happens with the tests.
Comment #32
TR CreditAttribution: TR commentedCore Entity API tests have now been fixed. I retested the patch in #28 and as you can see there are no problems with it.
Committed #28. Thanks for working on this.