Notice thrown if option 'external' not set

Posted by kndr on June 4, 2009 at 11:57am

Issue Summary

In function globalredirect_init() option 'external' should be set to FALSE (I think) since following message is shown: "There is a notice: Undefined index: external in language.inc on line 109." (locale and i18 module are enabled). Look at core url() function, where language_url_rewrite() is called - 'external' option is always set before call to language_url_rewrite();

Attachment	Size	Status	Test result	Operations
external.patch	618 bytes	Idle	Failed: Failed to apply patch.	View details

Comments

#1

Posted by smk-ka on July 14, 2009 at 4:32pm

Status:

needs review

» reviewed & tested by the community

Yes, this is annoying. Re-rolled patch with nothing else than whitespace fixed, so it's RTBC.

Attachment	Size	Status	Test result	Operations
481856-notice-D6.patch	860 bytes	Ignored: Check issue status.	None	None

#2

Posted by ranza on July 23, 2009 at 11:16am

i actually just fixed line 109 by doing if(!isset($options['external'])) {} insted of !$options['e... in language.inc

#3

Posted by mrfelton on October 3, 2009 at 10:37am

That sounds like perhaps a patch to core is what is really needed.

#4

Posted by smk-ka on October 4, 2009 at 11:44am

Title:	Option 'external' should be set before language_url_rewrite() is called	» Notice thrown if option 'external' not set
Project:	Global Redirect	» Drupal core
Version:	6.x-1.x-dev	» 7.x-dev
Component:	Code	» language system
Status:	reviewed & tested by the community	» needs review

Agreed, let's try that: language_url_rewrite() should not throw a notice if $options['external'] is not set.

Attachment	Size	Status	Test result	Operations
481856-notice-external.patch	630 bytes	Idle	Passed: 13660 passes, 0 fails, 0 exceptions	View details
481856-notice-external-D6.patch	642 bytes	Ignored: Check issue status.	None	None

#5

Posted by Damien Tournoud on October 4, 2009 at 11:49am

Erm. Why not fixing that in Global redirect? You are calling a core function, it's your responsibility to call it correctly.

#6

Posted by sun on October 4, 2009 at 11:54am

Status:

needs review

» reviewed & tested by the community

Because it's bad code? $options['external'] is an optional property.

#7

Posted by Damien Tournoud on October 4, 2009 at 11:57am

$options['external'] is an optional property.

It is not, it is set by default in url(), before calling language_url_rewrite().

#8

Posted by Damien Tournoud on October 4, 2009 at 12:00pm

Project:	Drupal core	» Global Redirect
Version:	7.x-dev	» 6.x-1.x-dev
Component:	language system	» Code
Status:	reviewed & tested by the community	» needs work

It makes zero sense not to fix that in Global redirect first.

#9

Posted by mrfelton on October 4, 2009 at 6:30pm

Nothing in http://api.drupal.org/api/function/language_url_rewrite/6 mentions that 'external' is a required option. Surely, just because the parameter is set in url() before the function is called, that doesn't make it a mandatory parameter - that's just the way that url() uses it.

EDIT: Also note that in http://api.drupal.org/api/function/url/6, where the various options are defined (on the language_url_rewrite page it says that the options are the same as for url()) the entire $options array is optional, and if (!isset($options['external'])) is used the check for the presence of the 'external' option.

#10

Posted by eMPee584 on January 6, 2010 at 3:37pm

Priority:	normal	» critical
Status:	needs work	» reviewed & tested by the community

Just installed this module, and with more than 26 kilo users, surely this module will not throw a PHP notice on EVERY page access, right?

Why has the maintainer still not committed this simple fix?

i was just about to submit this patch too.. Surely the check in core's language_url_rewrite should be fixed aswell (as mrfelton rightly stated, the docs don't mention that 'external' is required....) but first and foremost the problem is caused by the globalredirect module...