[D7] Sticky Sharrre Bar

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxRuslanPiskarev2268241git

I'm a robot and this is an automated message from Project Applications Scraper.

Hi,

Thanks for your contribution.
Here is my review result

in .install file:
- hook_enable() is empty, if not needed, it can be simply deleted.

- wrong documentation for sticky_sharrre_bar_uninstall() , this is hook_uninstall not hook_install.

- 'title' => 'Waypoints library', this needs get_t()

- same need of get_t() for 'unknown' in the value key

-same notice for 'title' => 'Sharrre library',

in .module
- in hook_block_info

'region' => 'header',
'visibility' => BLOCK_VISIBILITY_NOTLISTED,

i don't think all themes have a 'header' region also for some theme the 'to_header' may be more appropriate. so i suggest to let the block listed and not forcing to header regin if it doesn't exist.

- '#title' => t('Use the css of module.'),, i think a better wrding is 'Use the css of the module' ou "Use the module's css"

-

if (!empty($enabled_providers) && sticky_sharrre_bar_libraries_loaded() && user_access('access sticky_sharrre_bar')) {
sticky_sharrre_bar_libraries_loaded();

i don't think we need the second sticky_sharrre_bar_libraries_loaded(); do we ?

- $node_url = $base_root . request_uri(); and 'node_url' => $node_url,
the variable name may lead to a misunderstanding as $base_root . request_uri(); will not always be a node url, it may be anything else as a page manager or a view...it only depends on the block visibility.

- in theme_sticky_sharrre_bar_block() instead of hard coding ul and li, theme('item_list', $vars) can be used and instead of hard coding h2, theme('html_tag', $vars); can be used

- it would be great to have a way for modules to add new providers and a documentation for it, but this may be a feature request.

in sticky_sharrre_bar.js
- i don't think it's a good idea for this odule to do anything related to googleanalytics and enableTracking stuff, we should let the googleanalytics module take care of this. or at least, document it and have an option to choose to care of it or not.

Thanks again !

Moving back to needs work for the use of $base_root issue.

.gitignore file should be removed.

.install file:
It is common to assign get_t() to $t().

.module file:
There is inconsistent use of single vs double quotes. Please use single quotes, its faster and more consistent.

sticky_sharrre_bar_block_view
Use of $base_root should be replaced with libraries_get_path() to return the path. The current approach won't work on multi-lingual sites.

theme_sticky_sharrre_bar_block()
Don't call theme() directly. Instead pass up the render array so that page alters can still adjust the rendering of the block.

Here's some great information on render arrays: https://drupal.org/node/930760

Manual Review

1. You have a 7.x-1.0 branch. This needs to be removed. A release version will be a tag.
See https://drupal.org/node/711070#branches-tags

2. You have SASS, but no config.rb to define your environment. You are also not using any nesting in your .scss file.

3. Your attach in sticky_sharrre_bar.js is not defining the second argument, settings, which should be
used instead of Drupal.settings (well, you are using this in one place by it will be undefined). You should then use this instead of Drupal.settings

5. sticky_sharrre_bar.js:14-15: This variable will be nuked because it is local scope.

6. Indent with two spaces in sticky_sharrre_bar.js See https://drupal.org/node/172169#indenting

7. In sticky_sharrre_bar_block_view(), use #attached instead of drupal_add_css() and drupal_add_js().

8. sticky_sharrre_bar_block_view:46: You are using variable_get without the default value. Core uses 'bartik' for this.

9. sticky_sharrre_bar_block_view:158: Avoid using $base_path directly if you can.

10. sticky_sharrre_bar_block_view:158: You are using variable_get without the default value. Core uses 'Drupal' for this.

11. There are a few more variable gets w/o default values.

12. theme_sticky_sharrre_bar_block() should really be replaced by a .tpl.php file. Reworking to use a proper render array would be OK, too, but I think a template would really be better in this case.

Automated Review

FILE: /var/www/drupal-7-pareview/pareview_temp/css/sticky_sharrre_bar.css
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
121 | ERROR | CSS colours must be defined in lowercase; expected #fff but
| | found #FFF
--------------------------------------------------------------------------------

Manual Review

Where is icons.psd and icons.png come from? Did you make this, or it it third-party? Not really sure the PSD is needed in the repo.

sticky_sharrre_bar_block_view() has a variable_get without a default value on line 152.

Using #attached instead of drupal_add_js() and drupal_add_css() isn't a blocker, but a strong suggestion.

Setting back to Needs Work pending an answer about the icons, as third-party code / licensing issues can be show stoppers. The rest are suggestions.

variable for sticky_sharrre_bar_use_google_analytics_tracking should check for google_analytics module using module_exists() around line 101. And adjust other places in the module as necessary.

Ready for a git admin to review now. Marking as RTBC.

Sorry for the delay; we are working throught the queue as fast as we can.

Automated Review

Review of the 7.x-1.x branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives, and may be duplicate results from Coder Sniffer. See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

Minor problem with one docblock.

Manual Review

I would hold off on the 7.x-1.0 tag for a little while to make sure nobody has any problems.

Your JS behavior needs to use the context in the $.each()

(*) Line 22 of the behavior seems to reference a file / path that is not accounted for by the library. It also defines a PHP
path that would be handled outside Drupal that would be accessible to those without the permission to view the block, so this is a bit of a security problem. Remove this option, and update the README that only the
JS should be added.

This module is good to go once (*) is taken care of.

Forgot attachment.

Needs review is the correct status. You never set RTBC yourself.

As for the third party library "sharrre.php" that is loaded by your JS from a remote location, that is indeed a security hazard. There are guidelines for 3rd party code. Your module need to conform to these guidelines.

Review of the 7.x-1.x branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  FILE: /home/klausi/pareview_temp/js/sticky_sharrre_bar.js
  --------------------------------------------------------------------------------
  FOUND 10 ERRORS AFFECTING 4 LINES
  --------------------------------------------------------------------------------
    1 | ERROR | [ ] Missing file doc comment
   16 | ERROR | [ ] Inline comments must start with a capital letter
   16 | ERROR | [ ] Inline comments must end in full-stops, exclamation marks, or
      |       |     question marks
   16 | ERROR | [x] Comments may not appear after statements
   18 | ERROR | [ ] Inline comments must start with a capital letter
   18 | ERROR | [ ] Inline comments must end in full-stops, exclamation marks, or
      |       |     question marks
   18 | ERROR | [x] Comments may not appear after statements
   20 | ERROR | [ ] Inline comments must start with a capital letter
   20 | ERROR | [ ] Inline comments must end in full-stops, exclamation marks, or
      |       |     question marks
   20 | ERROR | [x] Comments may not appear after statements
  --------------------------------------------------------------------------------
  PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  --------------------------------------------------------------------------------
  

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. _sticky_sharrre_bar_parse(): why do you need cURL and cannot use drupal_http_request()? Not all PHP installations come with cURL.
2. "$output['count'] = str_replace('>', '', $filtered->item(0)->nodeValue);": why is the str_replace() needed? Please add a comment.
3. ticky_sharrre_bar_block_view(): using $base_root is wrong here, what do you want to do? I think you just want to do url(current_path(), array('absolute' => TRUE));?

But that are not application blockers, otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to stBorchert as he might have time to take a final look at this.

Setting back to "Needs work". I'm really sorry about this, but believe having third party content in the repo is a blocker.

3rd party code/content

No:

The theme/module comes bundled with a file named icons.png. As is it stated in #15, this file is a derivative work created from a pack downloaded from http://sensationalfix.com/flat-social-icons-eps/.

The in question pack includes the copyright infosheet shown below:

Hence, icons.png is third party content. Third party content is not generally allowed on Drupal.org and should be deleted.
This particular asset is made available by its author for reuse under the Creative Commons Attribution-ShareAlike (CC BY-SA) license. While it is perfectly legal under this license to create a derivative distribute it under CC BY-SA (provided the license is honored, see below), the CC BY-SA license is not tolerated on Drupal.org. All assets hosted on Drupal.org must be licensed under the GPL V2+ license.
This policy is described in the 3rd party libraries and content on Drupal.org. It also appears in the Drupal Git Repository Usage policy you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms.

Using material under a Creative Commons Attribution-ShareAlike license requires the licensee to comply with the license terms. This has not been done here. As long as the license is violated, distributing the materials constitutes a copyright violation, which should not be taken lightly. To repair this breach of license, please make sure that all five attribution requirements are followed. Attribution need to be in README.txt, on the site when the module is installed (e.g. via a link in the footer area to the copyright infosheet), and at the site you put up the asset for download.

The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.

I think you should be able to resolve this by hosting icons.png off-Drupal.org (e.g. on Flickr, Instagram, or GitHub), and to provide download instructions (like you do with the other 3rd party assets required by the project).

The icons were discussed offline, and are not a blocking issue. Setting back to RTBC.

@mpdonadio,
care to explain what was discussed offline?

These icons are copyrighted, and only usable under the CC BY-SA license. That author includes a copyright certificate to assert his rights, and he links to: http://www.dmrights.com/ . To me, it looks like the author of these icons actually care about his copyright.

Then there is the Drupal Git Repository Usage policy, which does not allow third party content licensed under the CC BY-SA license.

So to sum up: this project in its current state breaks copyright law and the Drupal Git Repository Usage policy.

Please also see this thread: https://www.drupal.org/node/2186215 (which is about CC BY - a license far more forgiving than CC BY-SA).

@Ruslan Piskarev,
the icons look very nice and it is perfectly legal to use them in FLOSS projects such as yours as long as you use them according to the license (i.e. attributes the author).

The main problem here as that in your project, they're not used according to the license and therefore: You violate the author's copyright.

Of course, if you prefer to create your own icons/buttons, that is up to you. Not using third party content is certainly a valid solution to this issue. But to me, it looks like a needless duplication of effort (and IMHO, it wll be hard to create social icons that look as polished as these).

Regarding the discussion mpdoandio had with me on IRC: I think it is fine to have derivative work of CC-SA in the drupal.org repositories, because the license is pretty similar. Otherwise the derivative work would have to be hosted elsewhere just for the sake of overly strict licensing enforcements, which does not make sense IMHO.

We should ask people to remove third party stuff from the repos if it is an unmodified copy (should only be hosted in one place) or if it is not GPL compatible at all (example: proprietary licenses that could really cause legal trouble for drupal.org).

No other objections for more than a week, so ...

Thanks for your contribution, Ruslan Piskarev!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

Regarding the discussion mpdoandio had with me on IRC: I think it is fine to have derivative work of CC-SA in the drupal.org repositories, because the license is pretty similar.

According to 3rd party libraries and content on Drupal.org, hosting third party materials on Drupal.org is the exception, and:

Any of the above exceptions needs approval by drupal.org administrators. Upfront: 90% of all exception requests are moot. Only ask for an exception if it is really required.

Has an such an exception been given in this particular case?

For the record: I agree with you! It should be fine to have such a derivative work in the drupal.org repositories.

However, that agreement is not universal. Please take a moment and have a look at the following conversation: #2298275: Proposal to amend Drupal Git Repository Usage policy to allow content licensed under Creative Commons

I've already said publicly that I think that what appears to be the current policy regarding third party content potentially very damaging to Drupal.org and probably also non-governable.

I also think that having policies, but treating them as optional when it is more convenient to do so, results in applicants being treated unequal.

For the record: I would be very happy to see these policies changed.