Module is not working with hardcoded URLs in Drupal 6.x

Here is the list of changes I've made to the module:
in _ext_link_page_replace added valid_url validation instead of only “http” validation and sanitized passed url with rawurlencode so hardcoded URLs are now acceptable by the module.
added tips to reset cache in admin section in order to apply modifications to the Link Page and Page Title.
Replaced old sanitize of $_GET['url'] with filter_xss and valid_url ceck provided by Drupal core.
Added Java Script redirect with jquery and jquery.timers.js file in order to use js redirect with countdown timer.

CommentFileSizeAuthor
#2 ext_link_page.patch8.59 KBAinur
ainur_ext_link_page.patch11.5 KBAinur

Comments

zzolo’s picture

zzolo commented
Category: bug » feature
Status: Needs review » Needs work

Note that these changes are not a bug. Thank you for the hard work. Some notes:

  1. You are adding information into the .info file that is automatically created by drupal's CVS. Please use CVS to check out the HEAD version (there is a CVS Instructions tab on each project. Then create a patch the correct way: http://drupal.org/patch/create
  2. The help text about cache (there is a typo there as well), should be put in the hook_help as a general help for that page.
  3. JS redirection needs needs to check for JS availability with $_COOKIE['has_js'] and then either use HTML redirect or JS.
  4. JS needs to be in a specific file and utilize Druapl behaviors.
  5. Handling of third-party library needs to happen with the Libraries module. We cannot put a third-party library in the module.

Thanks again. I look forward to your changes.

Ainur’s picture

Ainur commented
Status: Needs work » Needs review
StatusFileSize
new ext_link_page.patch8.59 KB

Thanks for the review.
1. OK, I think this time I did correctly the procedure
2. done
3. done
4. I'm not big jqeury user, can't find other ways to make the redirect.
5. I've removed the library and added instructions where to get it if jquery.timers.js doesn't exists on the help section of the module + if .js file doesn't exists module uses meta refresh redirect.
I've also corrected some stuff from my previews patch and added instructions how to prevent bots from indexing redirected urls.
I'm successfully testing the patch on my community website.

zzolo’s picture

zzolo commented
Status: Needs review » Needs work

Thanks for the new version of the patch. Here are my thoughts.

  1. You are using tabs and not spaces and overall you are not consistent with spacing (use 2 spaces instead of tabs). Please read and follow the Drupal coding standards (note that there are standards for JS and CSS as well). http://drupal.org/coding-standards
  2. JS needs to be in a separate file. This is not a jQuery thing. There needs to be an ext_link_page.js file that holds the JS logic. JS should not be inline. You will also need to utilize Drupal.settings which you use drupal_add_js() to send values to JS from PHP.
  3. On similar note, it seems like you should be able to remove logic on encoding and replacing the URL.
  4. I don't want to include a third party library in the module, unless it utilizes the Libraries module: http://drupal.org/project/libraries

I appreciate the patch and all your work, but this does need to be rethought and cleaned up so more.

zzolo’s picture

zzolo commented
Version: 6.x-1.1 » 6.x-1.2

Please note that the new version has a lot of changes and some of these ideas in them. Make sure you update your CVS copy to make patches against. I have still not put in the JS.

arithmetric’s picture

arithmetric commented
Status: Needs work » Closed (won't fix)

The 6.x branch is now closed for new features.