Bugfix for #64604

[iwalz]

https://bugs.php.net/bug.php?id=64604

[iwalz]

By changing this line to

} else {
    STR_FREE(diff);
    goto nohost;
}

It will start to fail with strings like http://www.example/img/test.png.
Because of that, ext/filter/tests/015.phpt is broken and needs a modification - which should be avoided if possible. Suggestions?

[yohgaki]

Your patch has hard coded TLDs.
As you may know, any TLDs can be added now.

It's better to come up with patch that handles any TLDs.

[iwalz]

This patch, as of now, uses a basic set of TLDs to use if a host matches a specific pattern. Without the free on the comment above, it is not required to match the hardcoded TLDs to be identified as a host. E.g. localhost is still matched as the hostname, without matching the pattern of the hardcoded TLDs. The hardcoded solution is exactly the reason, why I picked that way to fix this issue. Only something like index.php on the first place requires this kind of check, and this "abstract" solution allows to identify the most common ones.

In the real world, there's no way to really match the right pattern for sure. Even if you do a DNS lookup on the string that is "supposed to be the host", e.g. index.com could be matched as host, but can also bo part of the path for a java-common directory structure.

This patch basically fixed 2 issues with parse_url. The one with the hardcoded TLDs is one showing up in the comments. If the TLDs are a problem, I can split it up (the if on line 204 is simply at the wrong place) and create a new ticket for the 2nd issue in the comment to look for a more rock-solid solution.

[yohgaki]

Since DNS looks or any access to network resource is not preferred, how about add 2nd optional parameter that adds non standard TLD names if necessary?

Separating issue sounds good to me, one for fix "//" issue (merge from 5.4), another for fix non standard TLD issue (merge from 5.6)

Personally, I don't mind merge your patch as it is now if optional parameter is going to be added newer releases, but I tends to like introduce new features to released versions :) Could you post mail to [email protected] to ask opinion from others.

[yohgaki]

English please.

@iwalz is this merged?

[kaplanlior]

@yohgaki No, this wasn't merged (I checked 5.4 and above).

[yohgaki]

I think the patch itself is good.
We may add optional TLD from users 5.6 and up.

[kaplanlior]

@yohgaki Seems this was closed in favor of #533

[yohgaki]

@kaplanlior Thank you.
I updated the bug report and added the new PR.
Assigned myself so that I won't forget this.

[iwalz]

@yohgaki @mapthegod seems to have problems with his mail server since a while.
There seem to be an automatic reply from his mailserver, answering the mails from github that there might be a delay with email delivery up to 3 days. This message ends up as a github post, the problem started 4 months ago and spamed several things in php-src: https://github.com/mapthegod?tab=activity

It was no real comment.

[bukka]

I think that this could be fixed if someone logged into the php github account and block him... or report abuse if it does not help...

https://github.com/account/ignore_user/mapthegod

@johannes please please could you try it? :)

[bukka]

actually the link does not work from here, it needs to be done from his profile (setting drop down...) :)

[Tyrael]

I tried to do the following:

• change to the php organization from the context switcher on the homepage
• go to the guys profile page
• block user

but when I switch back to my personal account, I still have him blocked, so I suspect that the block user feature always uses your primary account. :(

[bukka]

@Tyrael I have reported abuse (linking this thread...) and they got back to me:

From: "James Dennes (GitHub Staff)" <[email protected]>

I've blocked the mapthegod on behalf of the php organization.

Currently only GitHub staff can block a user on behalf of an organization.

Cheers,

James

So it should be hopefully fixed now! ;)

[Tyrael]

\o/