Instead of having a custom drupal_static in this function (\Drupal\sharethis\SharethisManager::sharethisIncludeJs) IMHO this code should use a property on the class like $sharethisJS and store the result of the calculation in there.

CommentFileSizeAuthor
#7 sharethisIncludeJs_should_cache_its_result-2769903-7.patch1.24 KBmrinalini9
#3 sharethisIncludeJs_should_cache_its_result-2769903-3.patch1.95 KBguptahemant
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer and at Acquia for Acquia commented
Parent issue: » #2764639: Sharethis - 8.x-2.0-beta4
guptahemant’s picture

guptahemant CreditAttribution: guptahemant as a volunteer and at QED42 commented
Status: Active » Needs review
FileSize
sharethisIncludeJs_should_cache_its_result-2769903-3.patch1.95 KB

I have created a patch to replace drupal_static with $sharethisJS variable. please review.

gg24’s picture

gg24 CreditAttribution: gg24 as a volunteer and at QED42 commented
Status: Needs review » Reviewed & tested by the community

Hi @guptahemant,

This patch applies cleanly and I am able to see Sharethis service working fine.

The steps followed:

  1. Applied patch.
  2. Sharethis functionality works as expected.
  3. Tried removing and adding new services from the list and saved.

Hence works fine. Moving this to RTBC.

Thanks!

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch failed to apply

mrinalini9’s picture

mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commented
Assigned: Unassigned » mrinalini9
mrinalini9’s picture

mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commented
Assigned: mrinalini9 » Unassigned
Status: Needs work » Needs review
FileSize
sharethisIncludeJs_should_cache_its_result-2769903-7.patch1.24 KB

Rerolled patch for 8.x-2.x, please review.

Greg Boggs’s picture

Greg Boggs
he/him
Primary language English
Location Portland Oregon
CreditAttribution: Greg Boggs commented
Status: Needs review » Reviewed & tested by the community

Reroll looks good. Thanks Mrinalini!

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs reroll

Committed and pushed to 8.x-2.x

Status: Fixed » Closed (fixed)

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