CommentFileSizeAuthor
#28 interdiff_26-28.txt725 bytesanagomes
#28 3124369-28.patch4.96 KBanagomes
#26 interdiff_2-26.txt1.94 KBanagomes
#26 3124369-26.patch4.37 KBanagomes
#22 interdiff_20_21.txt757 bytessanjayk
#21 interdiff_18_21.txt0 bytessanjayk
#21 3124369-21.patch4.87 KBsanjayk
#20 interdiff_18_20.txt757 bytessanjayk
#20 3124369-20.patch5.35 KBsanjayk
#18 interdiff_15_18.txt4.82 KBsanjayk
#18 3124369-18.patch5.35 KBsanjayk
#15 3124369-15.patch5.46 KBPooja Ganjage
#12 3124369-12.patch7.12 KBPooja Ganjage
#8 Implement_Entity_t_method_3124369.patch8.66 KBPooja Ganjage
#7 interdiff_4-7.txt1.32 KBDeepak Goyal
#7 3124369-7.patch4.27 KBDeepak Goyal
#4 interdiff_2-4.txt4.98 KBsanjayk
#4 3124369-4.patch4.74 KBsanjayk
#2 3124369-2.patch1.92 KBswatichouhan012
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swatichouhan012 created an issue. See original summary.

swatichouhan012’s picture

Assigned: swatichouhan012 » Unassigned
FileSize
1.92 KB

Kindly review patch.

sanjayk’s picture

Assigned: Unassigned » sanjayk
sanjayk’s picture

I 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.

Berdir’s picture

Status: Needs review » Needs work
+++ b/tests/modules/entity_module_test/entity_module_test.module
@@ -4,12 +4,12 @@
  */
 function entity_module_test_entity_bundle_info() {
-  $bundles['entity_test_enhanced']['default']['label'] = t('Default');
-  $bundles['entity_test_enhanced']['first']['label'] = t('First');
-  $bundles['entity_test_enhanced']['second']['label'] = t('Second');
-  $bundles['entity_test_enhanced_with_owner']['default']['label'] = t('Default');
-  $bundles['entity_test_enhanced_with_owner']['first']['label'] = t('First');
-  $bundles['entity_test_enhanced_with_owner']['second']['label'] = t('Second');
+  $bundles['entity_test_enhanced']['default']['label'] = $this->t('Default');
+  $bundles['entity_test_enhanced']['first']['label'] = $this->t('First');
+  $bundles['entity_test_enhanced']['second']['label'] = $this->t('Second');
+  $bundles['entity_test_enhanced_with_owner']['default']['label'] = $this->t('Default');

this is not a class.

Deepak Goyal’s picture

Assigned: Unassigned » Deepak Goyal
Deepak Goyal’s picture

Assigned: Deepak Goyal » Unassigned
Status: Needs work » Needs review
FileSize
4.27 KB
1.32 KB

Hi, @Berdir
Updated patch please review.

Pooja Ganjage’s picture

Hi,

I am applying patch with using \Drupal\Core\StringTranslation\StringTranslationTrait class.

Please review once.

Pooja Ganjage’s picture

Berdir’s picture

Version: 8.x-1.0 » 8.x-1.x-dev
Status: Needs review » Needs work

The patch doesn't apply.

  1. +++ b/modules/contrib/entity/src/Form/RevisionRevertForm.php
    @@ -41,10 +52,14 @@ class RevisionRevertForm extends ConfirmFormBase {
        */
    -  public function __construct(DateFormatterInterface $date_formatter, EntityTypeBundleInfoInterface $bundle_information) {
    +  ¶
    +  public function __construct(DateFormatterInterface $date_formatter, EntityTypeBundleInfoInterface $bundle_information, TranslationInterface $string_translation) {
         $this->dateFormatter = $date_formatter;
         $this->bundleInformation = $bundle_information;
    +    $this->stringTranslation = $string_translation;
    

    this seems to add an extra empty line with spaces.

  2. +++ b/modules/contrib/entity/src/UncacheableEntityPermissionProvider.php
    @@ -42,6 +45,35 @@
    +      $container->get('string_translation')
    +    );
    +  }
    +  ¶
       /**
    

    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.

sanjayk’s picture

Assigned: Unassigned » sanjayk
Pooja Ganjage’s picture

Hi,

Attached updated patch.

Kindly review this patch once.

Thanks.

Pooja Ganjage’s picture

Status: Needs work » Needs review
sanjayk’s picture

Assigned: sanjayk » Unassigned

@Pooja Ganjage have same issues "No such file or directory" in both #8 and #12 please fixed it.

Pooja Ganjage’s picture

Hi,

Attached updated patch.

Kindly review once.

Let me know for any correction.

Thanks.

Pooja Ganjage’s picture

Needs Review

sanjayk’s picture

@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.

sanjayk’s picture

Uploading correct patch.

Status: Needs review » Needs work

The last submitted patch, 18: 3124369-18.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

sanjayk’s picture

Sorry by mistake uploaded wrong patch

sanjayk’s picture

@Berdir by mistake added test for D9.1 but test cases pass for D8.

sanjayk’s picture

FileSize
757 bytes

Please ignore #21 attached interdiff.

sanjayk’s picture

Status: Needs work » Needs review
TR’s picture

Status: Needs review » Needs work

This 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.

anagomes’s picture

Assigned: Unassigned » anagomes
anagomes’s picture

Assigned: anagomes » Unassigned
Status: Needs work » Needs review
FileSize
4.37 KB
1.94 KB

I'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.

Status: Needs review » Needs work

The last submitted patch, 26: 3124369-26.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

anagomes’s picture

Status: Needs work » Needs review
FileSize
4.96 KB
725 bytes

Uploading a new patch with small corrections, since the last one didn't pass the tests.

Status: Needs review » Needs work

The last submitted patch, 28: 3124369-28.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

TR’s picture

Status: Needs work » Needs review

Note 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.

  • TR committed 2344c65 on 8.x-1.x authored by anagomes
    Issue #3124369 by sanjayk, Pooja Ganjage, anagomes, Deepak Goyal,...
TR’s picture

Status: Needs review » Fixed

Core 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.

Status: Fixed » Closed (fixed)

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