| Project: | Drupal.org CVS applications |
| Component: | Miscellaneous |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
Hello guys,I'm working as full time Drupal Developer and I want to commit my first module. And start to contribute back to the community.
My module it's called Topsy Retweet button. It's a widget that offers you to retweet the nodes easily. More info about it and screenshots:
http://labs.topsy.com/button/
As you see exists for Wordpress but still nothing implemented on Drupal
http://drupal.org/search/apachesolr_search/topsy
Features
------------------------------
* A Twitter retweet button that shows tweet counts and allows retweeting.
* Highly customizable buttons can be displayed in various colors.
* Automatic URL shortening using bit.ly, (soon including support for others)
* Designed for high volume sites.
* Displays Topsy TopLinks badges for posts that are in the Topsy Top5k or above - http://labs.topsy.com/toplinks
* Topsy doesn't delete old tweets, so the plugin will work on all of your historical posts.
* Allows you to choose the username used as the source for retweets from your blog - e.g. "RT @ ..."
* Buttons are enabled per content type.
* Separated settings (size, position, css of the button) for teaser / full node.
* No module dependencies. (It needs jquery and works fine with the old library of Drupal core, it doesn't need jquery_update)
Sponsor
---------------------------
* Drupro
Rapid Drupal Development.
http://drupro.fi
Other modules
-------------------------------
Only similar module I found is Tweetmeme that provides a retweet button *but* is from other service.
http://drupal.org/project/tweetmeme
About the code
----------------------------
* It's ready to commit. It's not an idea is already coded.
* I'm careful about the Drupal standards and security standards and tried to stick with them. Anyway, I would appreciate a review.
* Coder 2.0 module doesn't complain about the code.
* It's heavily inspired on the Topsy Wordpress button and if it shares any code (specially js) it shouldn't matter because this code is also under GPL
That's all,
It's exciting moment for me in my Drupal career :D
I don't know how could I attach the code.
Should I wait for your reply ?
Cheers,
David Corbacho
Comments
#1
Module attached
#2
Hello and thanks for applying.
Reading the features, I noticed it also includes the short URLs feature. Is it a feature that is required for the module? There is already a project for that.
#3
Thanks for having a look and for the suggestion.
Yes, when you click "retweet" it will short the URL, but the process it's not done in the module.
It's done by the topsy service, at the same time that they redirect you to twitter.com. By default they do it with bit.ly service.
The link is something like this (with empty parameters):
http://button.topsy.com/retweet?nick= &url= &title=Good news are:
If you include the short URL in the link of the retweet button, they will respect it and take that instead of generating it.
http://button.topsy.com/retweet?nick= &url= &shorturl= &title=So I will suggest in the project page of the module to use Shorten module for this task. (If they don't like bit.ly)
It will be an optional module (no dependencies).
I will work for this feature, but ....Should this extra feature stop this module for being committed?
PS. An example of this module: http://drupro.com/blog
#4
Thanks for your reply.
I understand now that the module exposes Topsy services, which include shortening URLs. This is quiet different from modules that allow to create short URLs making direct calls to bit.ly, or other shortening services. What you reported makes the difference clear.
#5
I hope a volunteer rescue this forgotten application and review it. Thanks.
#6
Generally good layout and formatting.
Adequate docblocks.
Attempted use of theme functions. Proper use of FAPI.
Nice that you provide some display options - block, link,
The use of variables seems messy, with many different dynamically-named ones invented on the fly. I see you at least try to tidy them up, but perhaps it would be better if all those settings were in a structured array, variables can be arrays you know. This would massively clean up your code variable_get()s everywhere too.
1. String concatenation in looks awkward
<?php/**
* Implementation of hook_help().
*/
function topsy_help($path, $arg) {
switch ($path) {
case 'admin/settings/topsy':
$output = '<p>'. t('The button displays the total number of tweets associated with your content, providing also a link to retweet it.') . '</p>';
$output .= '<p>' . t('In this page you can edit only the general settings of the button. To <strong>enable the Topsy retwet button</strong> and modify other settings <a href="@edit">edit the content types</a>.' , array('@edit' => url('admin/content/types'))) .'</p>';
$output .= '<p>'. t('You can check on Topsy\'s status on <a href="@status">status.topsy.com</a>.', array('@status' => url('http://status.topsy.com/'))) .'</p>';
return $output;
}
}
?>
.. but I don't know what a tidier solution is. You've gone to some lengths to use 't()' cleanly and avoid html in it. I think you and the translators be better off with treating the whole block as one string:
<?php$output = t('
<p>The button displays the total number of tweets associated with your content, providing also a link to retweet it.</p>
<p>In this page you can edit only the general settings of the button. To <strong>enable the Topsy retwet button</strong> and modify other settings <a href="@edit">edit the content types</a></p>
<p>You can check on Topsy\'s status on <a href="@status">status.topsy.com</a>.</p>',
array(
'@status' => url('http://status.topsy.com/'),
'@edit' => url('admin/content/types')
)
);
?>
... but don't take this as policy - it's just how I've found it easiest to read and maintain.
2. position of the widget
<?php$node->content['topsy_button'] = array(
'#value' => topsy_build_button($node, $format),
'#weight' => variable_get('topsy_' . $format . '_weight_' . $node->type, -20),
?>
You can use CCK hook_content_extra_fields() to do this and integrate with the CCK UI a lot better.
It may not apply if you don't use CCK at all, so not sure if your version is entirely redundant or what.
See hook_content_extra_fields() anyway.
3. theme_topsy_global_settings() writes javascript inline
<?phpdrupal_set_html_head(theme_topsy_global_settings()); // Not using drupal_add_js because there is no way to add a CSS id in <script> tag
?>
That's what drupal_add_js($data, $type='setting') is for.
If you need to communicate settings to your script, set it in settings (ends up in the var Drupal.settings) then read it from your script.
If you script is entirely just a collection of settings (as this one seems to be) then drupal_add_js($data, $type='inline') might be appropriate.
As it's a bridge module to another library, I can see why the js you produce may not be 100% Drupal style, and matches the expectation of the external library. Still, those js variables have been defined in the global scope, so may interfere with - or be interfered by other javascript. Maybe. It's a little hairy.
4. Wrong use of theme func
<?phpreturn theme_topsy_button($which_class, $theme, $css_opts, $coded, $inner_script);
?>
Try
<?phpreturn theme('topsy_button', $which_class, $theme, $css_opts, $coded, $inner_script);
?>
...
Security - there is deliberate inclusion of a third-party javascript, so that probably means potential for XSS from that domain. But that's how these widgets work. Not sure what to think about that.
#7
I am changing status as per previous comment.
#8
Thanks for reviewing it dman
I have gone through almost same dilemmas than you. I have tried to adapt the widget to Drupal-way as much as I could.
0. Lots of variables. True, When I was adding more granulated control of the position of the widget, variables names has been gotten a little more complex, but still they are very tidy. If you see comment (core) module has like 8 or 9 variables per node type, similar way 9 variable per content type fivestar module. So 7 variables per content type I don't think is crazy for a drupal module. Anyway, I agree with you and I will organizing them in arrays and get rid of many variable_get's.
1. Only one block of text in t() is visually cleaner and tidier, but I would prefer to leave html tags out. It's the way that help hooks are implemented in core modules and most of other important modules.
2. As I saw on other widget modules like "fivestar" they have both ways implemented. (They complement each other) CCK or adding the widget to the node->content.
It's in my plans to add CCK alternative, but this first version I wanted to do it lightweight so it's easier to review and test.
3. As you said, it's a third-party library, so I don't have control over how their widget works. The widget (the javascript file) is loaded from external location, so I can't modify its code.
If the widget is expecting to find the variables as Global, and not under Drupal.settings, what can I do?
I know that drupal_add_js will be the perfect solution (also because this way Javascript will be aggregated and will optimize performance). But the settings needs to be as global variables, and the tag needs to have the id="topsy_global_settings", something not possible to control with drupal_add_js arguments.
The only thing I could do is to put that piece of javascript in the footer hook, but then it will there in every single page, even if there is no widget.
As I did it, with drupal_set_html_head(), is only called when the widget is built.
What can be the best solution? Any ideas?
4. True, I'm using now theme hook correctly. Thanks
I will put the new version of the module this week.
#9
OK, I'd say my issues are addressed.
I'd say the code (and response) demonstrate proper awareness of "The Drupal Way" and the slight deviations were due to requirements of the third-party library.
It's true that some modules created a bunch of dynamically-named variables, but if every contrib did so we'd really be in trouble. I'm suggesting grouping them mainly because it would be better, easier to manage code.
I can't really suggest how to rewrite that js inclusion without totally groking what it is doing. It just looked less-then-optimal and worth revision if possible.
I give this a tentative thumbs-up, pending anyone else's comments. None of my issues here should be blockers.
#10
I'm excited for this module. Hope this CVS application goes through soon and we can see this module show up in the repository!
-Jeff
#11
#12
Thanks dman and Jeff.
I have attached an updated version. Nothing really new, only I fixed some bugs and some descriptions.
Also some settings have been combined so there is fewer of them.
After try to save settings of the module in arrays, I realized that admin settings are processed by http://api.drupal.org/api/function/system_settings_form_submit/6 which cares to handle and save the form values in variables table (1 variable per field), so I don't have control on them. If there is an alternative to group settings variables, I ask for advice or to point me a module who does it (I didn't found)
I wait for your final aprove.
#13
It's true;
system_settings_form()use a single variable for each form field, but there is a way to group the content of some form fields under the same Drupal variable.<?php
$form['example_settings'] = array(
'#tree' => TRUE,
);
$form['example_settings']['email'] = array(
// Form field definition.
);
$form['example_settings']['user_id'] = array(
// Form field definition.
);
?>
In this case, the form field values will be saved as an array in the Drupal variable example_settings; the array indexes will be , and .
$form['example_settings']could be a fieldset, if you need, but that is not required; if you don't want the form fields to be grouped in a fieldset, then use$form['example_settings']as I wrote it.#14
Thanks kiam, looked like the perfect solution. I only check it very fast, but it seems not to work, the way system_settings_form_submit works:
UPDATE. This example is what happens in node_type_form. (It works correctly in system_settings_form). See post below.
$value = array_keys(array_filter($value));(...)
variable_set($key, $value);
array_keys make that the array indexes are not "email" and "user_id" but numeric indexes:
[0] => 'email',[1] => 'user_id',
So the values are not saved.
I will check later tonight if there is any walk around. It would be cleaner code using #tree
#15
Hi, Kiam
#tree works for system_settings_form, creating a variable that contains an array of settings,as you suggested.
But it doesn't work when saving the settings of node types (i.e. altering the node_types_form ), where is the bigger number of variables.
You can see yourself how are implemented differently the submissions:
http://api.drupal.org/api/function/node_type_form_submit/6
http://api.drupal.org/api/function/system_settings_form_submit/6
In case of node_type_form_submit, that array_filter is what breaks the functionality of #tree. Why? It seems an old decision of drupal core mantainers to treat correctly "checkboxes" form type. (More info about, and how was solved with an ugly hack for system_settings_form in Drupal 5)
So I have updated my code to create an array of settings for admin_settings.
Tell me if I need to upload again, but it's quite small change.
Thanks and I'm ready for more reviews
#16
(Edited last comment to make it clearer and bumping)
Anyone giving a final thumbs up?
#17
I don't think this settings array issue - that is only a suggestion about looking for the 'best' code solution - should hold up the application in any way. It's true that core does something like this, and is messy with it. (Also with theme settings)
Everything else shows good style, good behavior, following the guidelines etc. So the application is good, and shows due diligence.
And patience!
#18
Thanks dman, I appreciate it.
bump! (I will do it every 2 weeks more or less. This starts to be frustrating)
#19
Hi again,
It's time for a new version of the module with even more features! :D
I took even to further steps some suggestions that kiam and dman arised, improved support with other modules (CCK, shorten) and I did big changes refactoring the code and admin settings.
* Support for creating the short URLs with Shorten module.
* New option: Why use shortening when you can use mydomain.com/node/38 ? That is already quite short.
* All settings in 1 spot: admin_settings_form. Makes it easier to configure and also most of the settings are saved in arrays. Makes code more readable (thanks dman for the tip)
* More performance. 3rd part javascript library now is cached. (Inspired on google analytics technique)
* CCK field for the retweet button. (implemented as a sub-module). Providing a checkbox to enable/disable the retweet button per node and in the display options of the cck field you can choose "big" or "small" format.
* Added support for automatic twitter hashtags.
* Sanatized output, use of check_plain, check_url, l(), t(), url()
* Again good formated and documented code! (Passed the "coder" module test with option "minor" that shows all the warnings)
Someone giving a final approve? ;) Thanks
#20
Before I say anything else, let me just say I think this application should be approved. I have some minor things that could be worked on at a later stage, but they should not hold up approval of this application.
Oh, and hi, I'm the maintainer of the Shorten URLs module, the Tweet module, and some others.
Anyway:
- Typically the initial $Id$ comment should have a space between the comment indicator (; or //) and the $Id$ part.
- It would be nice if your README kept to 80 characters or less per line. Also true of function docblocs.
- Comments should generally be sentence case; there's one that isn't in topsy.install.
- topsy.admin.inc has a function called trim_and_polish() that needs to be namespaced.
- Use of the @param and @return indicators could be improved in docblocs.
- Any place you access an array parameter that might not be there (like any of the properties of $sett in theme_topsy_button()) you should always either check that it exists first using isset() to avoid E_ALL errors, or you should provide defaults for it using e.g. $sett += array('parameter' => $value). Not a big deal since your module always provides all the parameters, but someone else could use your API and the docbloc for it says that some parameters are optional.
- debug_backtrace() is very expensive in Drupal as far as function calls go because by the time you get to the endpoint you're usually 20 or so functions back. I don't quite understand what you're doing there but it seems like it would be much better to just pass whatever value you're looking up in as an optional parameter.
- line 381 the comment says "the a javascript", also should say "inspired by" not "inspired on"
- You should drop all the commented-out code at the bottom of topsy-admin.js to reduce the size of the file the user has to download
- topsy-admin.js would typically be called topsy.admin.js or more rarely topsy_admin.js
Seriously though, this module is well-written, and I'm big on stylistically correct code. I would approve this application.
#21
Thanks for taking time to review it Isaac.
- Fixed my .info file with an extra space in ;$Id$
- Line wrapping all doc to 80 characters and fixed that sentence case line.
- Renamed to topsy_trim_and_polish()
- Improved @param and @return of a couple of functions (may still need more improvement)
- Added default values to array of settings to prevent "undefined index" PHP errors. (Good catch!)
- debug_backtrace() is used to add a different css class depending of who is calling theme_topsy_button.
If is the own module: topsy_widget_data
If it's an external call: topsy_widget_shortcode
I will write an email to Topsy staff to see if it's so important this different class names and we can get rid of debug_backtrace(), passing it as a parameter (the only con I can think about it, is that you could fake it)
- Removed the commented javascript (I forgot it) from topsy-admin.js and
- Renamed to topsy.admin.js
- I appreciate all grammar/misspellings mistakes, because English is not my native language as you can see and they are the hardest bugs to find :)
New version is attached. Also fixed a bug (better call it feature) in the CCK module to show button by default, without need to edit old nodes.
#22
I am going to approve the application basing on the review done by IceCreamYou.
#23
Automatically closed -- issue fixed for 2 weeks with no activity.