New plugin: sharrre.com

Now that ShareMessage is short before getting pluggable, i'm excited to see this happen soon! :-)

1. +++ b/config/install/sharemessage.sharrre.yml
   @@ -0,0 +1,5 @@
   +services: {  }
   

   We should enable some default services.

2. +++ b/config/schema/sharemessage.schema.yml
   @@ -67,9 +89,6 @@ sharemessage.sharemessage.*:
   -    enforce_usage:
   

   Didn't properly rebase?

3. +++ b/css/sharemessage.sharrre.css
   @@ -0,0 +1,50 @@
   diff --git a/js/jquery.sharrre.js b/js/jquery.sharrre.js
   

   Is there no minified version?

4. +++ b/src/Form/AddthisSettingsForm.php
   @@ -125,7 +125,6 @@ class AddthisSettingsForm extends ConfigFormBase {
   -      ->set('message_enforcement', $form_state->getValue('message_enforcement'))
   

   Anothr revert?

1. +++ b/config/install/sharemessage.sharrre.yml
   @@ -0,0 +1,8 @@
   +shorterTotal: true
   
   +++ b/config/schema/sharemessage.schema.yml
   @@ -33,6 +33,28 @@ sharemessage.addthis:
   +    shorterTotal:
   
   +++ b/sharemessage.module
   @@ -127,5 +127,16 @@ function sharemessage_theme() {
   +        'shorterTotal' => TRUE,
   
   +++ b/src/Form/SharrreSettingsForm.php
   @@ -0,0 +1,109 @@
   +      ->set('shorterTotal', $form_state->getValue('shorterTotal'))
   +      ->set('enableHover', $form_state->getValue('enableHover'))
   +      ->set('enableCounter', $form_state->getValue('enableCounter'))
   +      ->set('enableTracking', $form_state->getValue('enableTracking'))
   
   +++ b/src/Plugin/sharemessage/Sharrre.php
   @@ -0,0 +1,159 @@
   +    $form['shorterTotal'] = array(
   ...
   +      '#default_value' => $this->getSetting('shorterTotal'),
   

   I think the config values are supposed to be lowercase underscore separated? (and so are the form keys.)

2. +++ b/css/sharemessage.sharrre.css
   @@ -0,0 +1,46 @@
   +.sharrre .box{
   ...
   +.sharrre .button{
   +    float:left;
   
   +++ b/js/sharrre.js
   @@ -0,0 +1,42 @@
   +          googlePlus: {size: 'tall', annotation:'bubble'},
   

   Codestyle (spaces) and lines below.

3. +++ b/js/sharrre.js
   @@ -0,0 +1,42 @@
   +  Drupal.behaviors.sharrre = {
   

   Prefix it with sharemessage_?

4. +++ b/js/sharrre.js
   @@ -0,0 +1,42 @@
   +      $('#sharemessage').sharrre({
   

   Oops... This is telling mit that things don't work when A sharrre and an addthis thing is present at the same time!

   Can you please extend the sharemessage_demo with placing a second message for Sharrre?

   I also realise that the demo does not cover images configuration...

5. +++ b/src/Form/SharrreSettingsForm.php
   @@ -0,0 +1,109 @@
   +    $addthis_config = $this->config('sharemessage.sharrre');
   
   +++ b/src/Plugin/sharemessage/Sharrre.php
   @@ -0,0 +1,159 @@
   + * AddThis plugin.
   ...
   +class Sharrre extends SharePluginBase implements  SharePluginInterface {
   

   Sharrre and not AddThis. And there are other similar locations.

6. +++ b/src/Plugin/sharemessage/Sharrre.php
   @@ -0,0 +1,159 @@
   +   * Gets the default AddThis settings.
   

   This does not return the "default" settings. It still contains the overrides. Only use the term "default" in an API if it is the global default.

7. +++ b/src/Plugin/sharemessage/Sharrre.php
   @@ -0,0 +1,159 @@
   + *   description = @Translation("Sharrre plugin for ShareMessage module."),
   

   This text is totally meaningless. If anything, then link to the provider and or describe what it is! Same applies for the existing AddThis description... ;-)

8. +++ b/src/Form/SharrreSettingsForm.php
   @@ -0,0 +1,109 @@
   +      '#title' => t('Format number like 1.2k or 5M'),
   

   And what if unchecked?

9. Followup: You introduce similarly to addthis the concept of global defaults with overrides... If this is the usual pattern, then we should make ShareMessage back this. Unsure. I'm not sure if default settings make sense here.
10. Followup: I realise if we override, all settings are empty. Instead we should initialise it with the global settings if no override is defined.

Still no idea why the test fails!

Is it possible that the AddThis plugin tries to connect to addthis to load the services definition and that fails with a timeout because the testbot does not allow external connections. As a result no services are available and none are output.

The module has a switch to use the local file for the definition. Then things should pass - or at least chances exist.

And we are missing sharrre specific tests. :-)

+++ b/src/Plugin/sharemessage/OGHeaders.php
@@ -0,0 +1,71 @@
+  public function calculateDependencies(){
...
+  public function getConfiguration(){

Codestyle :-)

+++ b/src/Tests/ShareMessageSettingsTest.php
@@ -124,4 +124,30 @@ class ShareMessageSettingsTest extends ShareMessageTestBase {
+    $this->assertRaw('<meta property="og:url" content="http://d8.dev/sharemessage-test/sharemessage_test_og_label" />');

Fix location - either determine the URL of the bot correctly by API or use assertPattern()

Yeah, time to get this in... :-)

Now you also provided here the OG plugin.. I thought we stay with the sharrre plugin and add the OG plugin in a separate issue #2600094: New plugin: open graph headers only
Why are we mixing now?

+++ b/sharemessage.module
@@ -30,6 +51,22 @@ function sharemessage_library_info_alter(&$libraries, $extension) {
+  $info = libraries_detect('sharrre');
+  // Self hosted library, use files from library definition.
+  if (!empty($info['files']['js'])) {

We should be describe a bit clearer about what is happening here.

+++ b/src/Tests/ShareMessageSettingsTest.php
@@ -81,12 +81,46 @@ class ShareMessageSettingsTest extends ShareMessageTestBase {
-  function testShareMessageDeleteCancel() {
+  function dtestShareMessageDeleteCancel() {

Hmm... skipped?!

Yeah we are almost there! ;-)

1. +++ b/js/sharrre.js
   @@ -0,0 +1,42 @@
   +          pinterest: {media: 'http://sharrre.com/img/example1.png', description: $('#shareme').data('text'), layout: 'vertical'}
   

   Example initialisation? ;-)

2. +++ b/src/Form/ShareMessageForm.php
   @@ -235,6 +248,33 @@ class ShareMessageForm extends EntityForm {
   +    if ($form_state->getValue('plugin') == 'sharrre') {
   

   That's not allowed here.
   Plugins need to check stuff on their own.

3. +++ b/src/Plugin/sharemessage/Addthis.php
   @@ -44,6 +44,9 @@ class Addthis extends SharePluginBase implements  SharePluginInterface {
   +        'tags' => $this->shareMessage->getCacheTags(),
   

   Thought we moved this into the builder?

4. +++ b/src/Plugin/sharemessage/Sharrre.php
   @@ -0,0 +1,218 @@
   +    static::isConfigured($form, TRUE);
   

   Then let's name it checkConfiguration()

5. +++ b/src/Tests/ShareMessageSettingsTest.php
   @@ -81,6 +81,40 @@ class ShareMessageSettingsTest extends ShareMessageTestBase {
   +    $this->drupalGet('admin/config/services/sharemessage/sharrre-settings');
   

   You want to have a test that checks the error message when configuration is missing.

Yay! Looks fine now.
And fine to cover more in followups...