Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gulab.bisht created an issue. See original summary.

gbisht’s picture

Status: Active » Needs review
FileSize
586 bytes

Created a patch with the alter to update or add new elements to the attribute variable.

gbisht’s picture

Patch for latest stable version 8.x-2.0-beta3

gbisht’s picture

FileSize
586 bytes

Improved patch for 8.x-2.0-beta3 version.

joshi.rohit100’s picture

Status: Needs review » Needs work

Inject module handler.

gbisht’s picture

Status: Needs work » Needs review
FileSize
2.72 KB

Patch updated and module handler injected.

gg24’s picture

Hi,

I am adding alter hook for attribute variable and data options both. As we are extending it for attributes, it is good to extend it for data options as well. Along with this I am adding sharethis.api.php as well which is missed in the above patch.

Please review.
Thanks!

purushotam.rai’s picture

Status: Needs review » Reviewed & tested by the community

In my opinion, it will be a good to have this option specifically, when more generalized. Marking it RTBC

Thanks and Regards

naveenvalecha’s picture

Status: Reviewed & tested by the community » Needs work

As the service provided by the module is public and there shouldn't be any BC break in its constructor, so any custom module decorated the existing service will be broken to add a new getter method and inject the module handler service by calling this explicitly in the constructor.

I'm fine with adding this F/R. but adding this as this seems to maintain an edge case.

purushotam.rai’s picture

@naveenvalecha could you please elaborate on what point u r stressing over here.

purushotam.rai’s picture

@naveenvalecha I guess, if any custom module uses this service, then there comes 2 scenario:
1. Service in Module Hook
2. Service in a Plugin/Another Service (PHP Class)

In both cases, we don't need to explicitly create arguments for the constructor of this service. Either we will use Drupal service or inject this into the class which definitely won't be broken.

Kindly let me know if we are on the same page or I'm missing something.

Thanks and Regards

purushotam.rai’s picture

Status: Needs work » Needs review
purushotam.rai’s picture

Assigned: Unassigned » naveenvalecha
purushotam.rai’s picture

I think container factory will take care of initialization.

purushotam.rai’s picture

Status: Needs review » Reviewed & tested by the community
andypost’s picture

I think it totally fine to change arguments of service, and module should allow to alter default render somehow

timodwhit’s picture

The patch works fine, but there seems to be an issue with the module when selecting which buttons to alter. Example: I needed to add specific text for a couple services, but as I could tell the service was not passed to this alter hook. Here is an example of what I had to to:

  $class = explode(' ', $attributes['class']);
  $class[] = 'fa';
  switch($attributes['displayText']) {
    case 'facebook':
      $class[] = 'fa-facebook';
      break;
    case 'twitter':
      $class[] = 'fa-twitter';
      break;
    case 'pinterest':
      $class[] = 'fa-pinterest';
      break;
    case 'email':
      $class[] = 'fa-envelope';
      break;
  }
  $attributes['class'] = implode(' ', $class);

There are couple things that are problematic here:

  1. Class is passed as a string
  2. The displayText is not a reliable thing to alter on
  3. No service awareness

Otherwise the patch is great, I think that the general theming aspect of this module needs a little work.

bibliophileaxe’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.77 KB

We had a requirement to render text in the span tag - specifically the title of the service. I have added the span text to the alter hook as well.

andypost’s picture

+++ b/src/SharethisManager.php
@@ -255,12 +266,18 @@ class SharethisManager implements SharethisManagerInterface {
+      $this->moduleHandler->alter('sharethis_render', $attributes, $data_options, $span_text);
+
       $meta_generator = array(
         '#type' => 'html_tag',
         '#tag' => 'span',
         '#attributes' => $attributes,
-        // It's an empty span tag.
-        '#value' => '',
+        '#value' => $span_text,

I still think that better to pass whole render element here to allow altering the build array

naresh_bavaskar’s picture

Assigned: naveenvalecha » Unassigned
Status: Needs review » Reviewed & tested by the community

#18 applied successfully and works fine for me.
unassigning @naveenvalecha..

ankitv18’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.84 KB

Re-roll the patch for latest version 2.0.0-beta5
Removing the unused statements from SharethisManager

Review patch #21

Status: Needs review » Needs work

The last submitted patch, 21: 2900290-sharethis-beta5-an_alter_required_for-21.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

ankitv18’s picture

Fixed the phpcs issues.
Patch #23

Status: Needs review » Needs work

The last submitted patch, 23: 2900290-sharethis-beta5-an_alter_required_for-23.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

ankitv18’s picture

Ignore the patch #23
Missed space, not caught up in phpcs

Patch #25

Status: Needs review » Needs work
ankitv18’s picture

Status: Needs work » Needs review
FileSize
3.79 KB

Removing the renderer plugin from service file and re-roll the patch to work with 2.0.0-beta5
Review Patch# 27

akalata’s picture

Updates hook_sharethis_render_alter() documentation as the $share_text is also alterable.

VladimirAus’s picture

Version: 8.x-2.x-dev » 3.0.x-dev
Status: Needs review » Needs work

Requires rerolling...

mrinalini9’s picture

Status: Needs work » Needs review

Patch #28 applied cleanly on the 3.0.x branch, so I think reroll is not needed.

Checking patch sharethis.api.php...
Checking patch sharethis.services.yml...
Checking patch src/SharethisManager.php...
Applied patch sharethis.api.php cleanly.
Applied patch sharethis.services.yml cleanly.
Applied patch src/SharethisManager.php cleanly.

Thanks & Regards,
Mrinalini

VladimirAus’s picture

Status: Needs review » Reviewed & tested by the community

Thank you! Committed! 🍸

VladimirAus’s picture

Status: Reviewed & tested by the community » Fixed

  • VladimirAus committed 43ad0d90 on 3.0.x authored by akalata
    Issue #2900290 by gulab.bisht, ankitv18, bibliophileaxe, gg24, akalata,...

Status: Fixed » Closed (fixed)

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