Download & Extend

Make Fontdeck parse the domain correctly

Project:@font-your-face
Version:6.x-2.x-dev
Component:Fontdeck (provider)
Category:bug report
Priority:normal
Assigned:Unassigned
Status:postponed
Issue tags:Novice

Issue Summary

If you're using a local development environment eg. local.dev:8888 then we only want "local.dev" to get sent. Patch included.

AttachmentSizeStatusTest resultOperations
fontdeck.patch1.36 KBIdleFAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fontdeck.patch. Unable to apply patch. See the log in the details link for more information.View details | Re-test

Comments

#1

Version:6.x-2.x-dev» 7.x-2.x-dev
Status:active» needs work

Thanks for the report and the patch. A few issues:

1) Let's fix the 7.x branch first, then backport.
2) Patches should be made following the instructions at http://drupal.org/patch/create
3) Rather than setting the value of $domain, then returning, I think we can skip a step and return the result of the preg_replace() directly.

#2

I don't have time to play patch tennis, so do with it what you will :)

#3

Title:Fontdeck does not parse the domain correctly» Make Fontdeck parse the domain correctly
Component:Code (general)» Fontdeck (provider)
Status:needs work» needs review

Here's it against 7.x-2.x-dev, with the following changes:

  • implemented sreynen's 3) of #1,
  • let's call host a host.
AttachmentSizeStatusTest resultOperations
fontyourface-Make_Fontdeck_parse_the_domain_correctly-1611902-3.patch1.54 KBIdlePASSED: [[SimpleTest]]: [MySQL] 9 pass(es).View details | Re-test

#4

Status:needs review» needs work

This isn't working for me. Specifically, parse_url($_SERVER['HTTP_HOST'], PHP_URL_HOST) is returning FALSE, I think because $_SERVER['HTTP_HOST'] is only host, not a full URL. $host = parse_url('http://' . $_SERVER['HTTP_HOST'], PHP_URL_HOST); works for me. That feels a bit hackish, but I don't have any better ideas.

#5

According to docs,

Partial URLs are also accepted, parse_url() tries its best to parse them correctly.

...but sometimes the best is just not enough.
As the whole purpose of this was to strip port numbers, it could as well read:
$host = parse_url($_SERVER['HTTP_HOST'], PHP_URL_PORT) > 0 ? parse_url($_SERVER['HTTP_HOST'], PHP_URL_HOST) : $_SERVER['HTTP_HOST'];
I did a couple of quick tests, strings like localhost:666 and 127.0.0.1:666 are parsed correctly.

#6

Or, for slightly shorter lines and to avoid calling parse_url() twice:

<?php
$details
= parse_url($_SERVER['HTTP_HOST']);
$host = isset($details['port']) ? $details['host'] : $_SERVER['HTTP_HOST'];
?>

#7

I did some more testing to try to figure out where parse_url() breaks down. It looks like it works consistently with a host and a port, but always fails with just the host. So I think #6 will work, though I'd change the isset($details['port']) to isset($details['host']). If it somehow did parse the host without a port, I think we'd still want the parsed host.

#8

Status:needs work» needs review

Implements #6 + #7, plus adds a somewhat verbose comment on what all this means.

AttachmentSizeStatusTest resultOperations
fontyourface-Make_Fontdeck_parse_the_domain_correctly-1611902-8.patch1.79 KBIdlePASSED: [[SimpleTest]]: [MySQL] 9 pass(es).View details | Re-test

#9

Status:needs review» fixed

Committed.

#10

Version:7.x-2.x-dev» 6.x-2.x-dev
Status:fixed» patch (to be ported)

#8 needs backporting to D6.

#11

Tagging.

#12

Status:patch (to be ported)» postponed

This may become obsolete when #1891820: fontdeck_get_domain() - stripping www is not necessary and #1894072: Protocol relative URLs for Fontdeck are resolved.