Word Link

This module seems similar to http://drupal.org/project/keyword_link, why not join forces and port it to D7?

Drupalrv,

Your sandbox project still contains a master branch. Refer to this link http://drupal.org/node/1127732 and remove master branch .

Also your project is Cross Site Scripting Vulnerable . When you add this text

to any of your form fields. It will simply output this text without sanitization resulting in Cross site scripting vulnerability. Read drupal security documentation http://drupal.org/writing-secure-code and make proper use of filters like check_plain(),filter_xss() etc while dealings with text which will be displayed as an output on screen.

Thanks.

It is good Drupalrv. This can prevent XSS. But It would be better if you apply those check_plain() filter inside function word_link_list_page() in file word_link.admin.inc. Have a look at http://drupal.org/node/263002. The values that will be displayed are contained in $words variable. So applying filter at this variable will be better.

I have not looked at your complete code. I will soon provide you more feedback regarding the same.

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

That doesn't sound like a reason to not make this a version 7.x-2.x version of Keyword Link. The only difference is the method you are using to accomplish the same thing, which could change from version 1.x to 2.x of Keyword Link (2.x being your version).

Sorry for being difficult, but it's one of the criteria for new projects that they don't duplicate existing functionality.

I agree. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the keyword_link issue queue to discuss what you need. You should also get in contact with the maintainer(s) to determine what is needed that this works for you.

If that fails for whatever reason please get back to us and set this back to "needs review".

So please follow the abandoned module process to take over the module. http://drupal.org/node/251466

If that fails for whatever reason please get back to us and set this back to "needs review".

did not mean to remove the review bonus tag.

Agreed RE Keyword link - conversion of a module is not too bad - use the coder module to look for the things which need to be converted.

The keyword link module is very small - and should be converted relatively easily.

Anyway - here's my review of your module as it is.

Review of worklink

Description

The description could be clearer. Replace with links? Deos that change the text - or does it make the word into a link? A big help would be an explanation of what this module is for - what need does it fulfil - why would someone want to use it.

Missing help page.

Not sure if this is a requirement - but it may be nice to have a help page accessible from the modules page.

Usage

This is starting from a pretty much vanilla D7 install.

When I added a wordlink I got two added to the list.

I then deleted one of them - I'd added a word link but nothing changed on the node - you need to put in an explanation here of how it works?

Sort of - I assume - add a word link - then go to the 'Word link settings' tab and set up the field which should have words converted.

As this point there is a functionality missing - what if I want to convert on more than one field? Surely the 'Word link settings' should have a multiple select list for the fields which can be seleected.

Also, word link only seems to work on certain field types - your docs and help page should explain which ones.

Big issue.

I added a field type of 'Text' and this field was not made available to have words converted to links.

Overall, the functionality seems to be OK - but better explained. Something like:

'Add Word Links to a list.
Then select which fields the conversion will occur on.'

Another possible enhancement is to be able to choose which word links should be applied to which field - i.e. in the word link settings page there should be a multiple select list which enables the user to select which conversions apply to a field. (Each field then may have it's own block).

Lastly - there may be a global option set for a word link - this may be seriously useful. I.e. if you have a wordlink list which - say - linked

Google - http://www.google.com
Drupal - http://drupal.org
Amazon - http://www.amazon.com

Then when users are adding text the words in the list would automatically be converted to links - which some sites might find useful.

Files review

README.txt

'The module allows you to replace certain words on the links.' is not really clear - probably should be

'This module allows you to set certain words to automatically be converted into web links. It is possible to build up lists of words which should be converted to links to specified URL's.'

'Replace word in content with a link' - it does not 'replace' - it 'converts' the word into a link.

Personally I would convert

'-- CONFIGURATION --

* Configure user permissions in Administration » People » Permissions:
Go to admin/people/permissions and grant permission to any roles
that need to be able to add and edit words.

* Configure content types and fields:
You can set node types, fields and replace limit at
admin/config/content/word-link.

* Add a new word:
Just follow the admin/config/content/word-link/add
link and fill out the form.
'

with

'-- CONFIGURATION --

* Configure user permissions in Administration » People » Permissions:
Go to admin/people/permissions and grant permission to any roles
that need to be able to add and edit words.

* Add a new word:
Just follow the admin/config/content/word-link/add
link and fill out the form.

* Configure content types and fields:
You can set node types, fields and replace limit at
admin/config/content/word-link.
'

The rest of the files look OK!

Conclusion

Looks generally OK - but could do with clearer explanations - and could probably do with the text field being available for wordlinks.

Also, to make it really useful there should be the option to apply the conversion to multiple fields in a node - and probably to be able to choose which conversion is applied to which field. Also, a global option might be useful.

Above poster did a good job at pointing out some contextual flaws, and I agree on most, Especially the README, which I think adds more confusion then it actually clears up.

Automated code review:
Codesniffer comes up completely blank. good job.

Manual review:
Looks solid. No glaring flaws, good use of api.

Conclusion:
From a coding stand point I have no problems approving this, however I agree with above poster that you need to improve quality of documentation first.

Removing review bonus tag, you have not done any manual review, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects.

Other than the aforementioned suggestions, it looks good and works as expected. It would be nice if it detected outside urls somehow, but maybe a 2.0 kind of thing.

Reviews of other projects

Great idea! It's the same to alinks module but more flexible.

1) I think you should to add setting to open external links in new tab/window.
2) UI: it would be great if you add special icon for external links to separate them from internal links.
2) word_link.info: you have to add field to dependencies[].
3) It's very minor but you should use <code> tag for your git instructions on project page

manual review:

1. word_link_menu_local_tasks_alter(): why do you need that? Shouldn't you define admin/config/content/word-link/add as MENU_LOCAL_ACTION in hook_menu()?
2. word_link_node_view(): "$words = word_link_get_link();": that is the same for all loop iterations, so it should be outside of the loop to avoid unnecessary queries.
3. word_link_replace_text(): do not use check_plain() on the title and class attributes in l(), that will be done in l() automatically. Same for check_url(), that is already done by l().
4. word_link_replace_text(): do not use mb_substr(), use drupal_substr() instead.
5. word_link_add_update_link(): use drupal_write_record() instead of db_insert/db_update and you get schema validation for free.
6. word_link_admin_settings_submit(): remove that function and use system_settings_form() in word_link_admin_settings() instead.

But that are not application blockers, so I guess this is RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

no objections for more than a week, so ...

Thanks for your contribution, drupalrv!

I updated your account to let you 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 get 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.

This doesn't work within panels which is a great shame. Anyone any idea how to tweak this so it can place links within an overridden node?

new reviews

@benchesters, #8084793 above.

Getting this to work with panels only requires you to comment out the code that determines whether the current path is a node/[nid] path and the checking for content type. I've attached a simple patch. This would ideally be worked into the module as a proper setting.

https://www.drupal.org/files/issues/1827294.patch

This project application is closed. Please post all feature requests to the issue queue of the module at https://www.drupal.org/project/issues/word_link?categories=All