steps to reproduce :
- enable local.module, add and enable a new language, say, "french" - no need to actually import a .po file
- configure language negociation to 'Path prefix only' (I did not test the other ones)
- go to any form page, using the fr/ path prefix, say fr/admin/settings/language/overview (really simple form)
- submit the form : you are redirected to admin/settings/language/overview (no fr/)

CommentFileSizeAuthor
#16 syslog_external.patch1.46 KBchx
#11 url_options_external-144634-11.patch2.47 KBchx
#9 url_options_external-144634-9.patch2.47 KBchx
#4 absolute_rewrite-144634-4.patch583 byteschx
#3 absolute_rewrite-144634-3.patch1.02 KBchx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

keith.smith’s picture

keith.smith CreditAttribution: keith.smith commented
Title: FAPI3 breaks language negociation » FAPI3 breaks language negotiation

(correcting title, I hope)

yched’s picture

yched CreditAttribution: yched commented

Right :). "Negociation" is, well, french.

chx’s picture

chx CreditAttribution: chx commented
Title: FAPI3 breaks language negotiation » Breaker: drupal_goto breaks language negotiation
Component: forms system » language system
Priority: Normal » Critical
Status: Active » Needs review
FileSize
absolute_rewrite-144634-3.patch1.02 KB

Just because you ask Drupal to slap $base_path onto your Drupal path you surely still want to do language URL rewrites. What you do not want to rewrite is an external URL thrown at url().

chx’s picture

chx CreditAttribution: chx commented
FileSize
absolute_rewrite-144634-4.patch583 bytes

THe second chunk was not needed.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

Yes, it seems like it was broken to check for the absolute flag, because there people ask url() to output an absolute URL. It does not say, that the actual "path" passed in was "absolute" = external. This surely needs to be tested with combinations of omitted language codes, domain name based setups, path prefix based setups and the like. In principal it looks like, so if testing shows this is finally right, then it is fine with me.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

yched: can you test?

yched’s picture

yched CreditAttribution: yched commented
Status: Needs review » Reviewed & tested by the community

Works (tested with 'path prefix' negotiation - unable to test 'domain name' negotiation on my localhost :-) )

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Needs work

menu_path_is_external is not particularly fast and calling this for every url() is NOT a good idea. We should add an 'external' flag in addition to the 'absolute' flag. All too often, people think that absolute == external.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs work » Reviewed & tested by the community
FileSize
url_options_external-144634-9.patch2.47 KB

Refactored. We already determine whether an URL is external or not, now that is called early and language_url_rewrite checks for that. I marked RTBC as there is no functionality change compared to the previous patch.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Needs work

- Is it a good idea to allow bypassing filter_xss_bad_protocol() by providing the external flag from outside? I'd say that this code was there, so we can just use it as an internal check. (For security's shake)
- The first documentation line you added is badly indented.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs work » Reviewed & tested by the community
FileSize
url_options_external-144634-11.patch2.47 KB

fxbp is only used for checking. security measures are taken by l in the form of a check_url. Spaces fixed.

Dries’s picture

Dries CreditAttribution: Dries commented

Patch looks good.

$ grep -r "http://" * | grep " l(" | grep -v @
modules/openid/openid.module:      '#description' => l(t('What is OpenID?'), 'http://openid.net/', array('external' => TRUE))
modules/syslog/syslog.module:      '!php'         => l("PHP's syslog", 'http://www.php.net/manual/en/function.openlog.php'),
modules/syslog/syslog.module:      '!syslog_conf' => l('UNIX/Linux syslog.conf', 'http://www.rt.com/man/syslog.5.html'),

Oddly enough, the OpenID link already used 'external'? Also, should we add an external attribute to the php and syslog_conf links?

chx’s picture

chx CreditAttribution: chx commented

can we do that in a followup patch and get this in? :)

Dries’s picture

Dries CreditAttribution: Dries commented
Title: Breaker: drupal_goto breaks language negotiation » Fix external links
Component: language system » base system
Priority: Critical » Minor
Status: Reviewed & tested by the community » Needs work

I committed this patch, and changed the subject. Thanks for your help, chx.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Fixed

Dries committed the patch.

chx’s picture

chx CreditAttribution: chx commented
Status: Fixed » Reviewed & tested by the community
FileSize
syslog_external.patch1.46 KB

Followup patch as agreed above.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Fixed

Great, committed.

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)