Comment on behalf of krakjoe at php.net:

labelling

Travis check fail seems to be unrelated.

Is there anything else I can do to get this merged? @weltling ?

@weirdan oh, i didn't follow this bug. Thanks for the ping, will check soon.

@weirdan the test doesn't pass with 5.6, but it was supposed to pass with it, right? Otherwise, there seems to be a mistake in the documentation, as the fifth argument is documented as pass-by-ref, which is not happening indeed. To the test, could you please give a hint?

Thanks.

tl;dr: the test works with 5.6, but it's fragile and may fail in your specific environment. The documentation is right

The test, unfortunately, is very sensitive to environment, as it queries the default DNS recursor for your host (could be your provider's, or your home router, or something else) and there's no easy way to change that. DNS servers have their own ideas on what to put to that additional section, and yours may have decided to not put anything. Here's, for example, two dig results, one using my office VPN and another one is from my direct home connection:
Office:

;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 25071
;; flags: qr rd ra; QUERY: 1, ANSWER: 1, AUTHORITY: 13, ADDITIONAL: 9

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 4096
;; QUESTION SECTION:
;php.net.			IN	MX

;; ANSWER SECTION:
php.net.		29	IN	MX	0 php-smtp2.php.net.

;; AUTHORITY SECTION:
net.			41501	IN	NS	l.gtld-servers.net.
net.			41501	IN	NS	b.gtld-servers.net.
net.			41501	IN	NS	g.gtld-servers.net.
net.			41501	IN	NS	j.gtld-servers.net.
net.			41501	IN	NS	h.gtld-servers.net.
net.			41501	IN	NS	f.gtld-servers.net.
net.			41501	IN	NS	e.gtld-servers.net.
net.			41501	IN	NS	d.gtld-servers.net.
net.			41501	IN	NS	i.gtld-servers.net.
net.			41501	IN	NS	c.gtld-servers.net.
net.			41501	IN	NS	a.gtld-servers.net.
net.			41501	IN	NS	m.gtld-servers.net.
net.			41501	IN	NS	k.gtld-servers.net.

;; ADDITIONAL SECTION:
c.gtld-servers.net.	67311	IN	A	192.26.92.30
d.gtld-servers.net.	51065	IN	A	192.31.80.30
e.gtld-servers.net.	26125	IN	A	192.12.94.30
f.gtld-servers.net.	17658	IN	A	192.35.51.30
h.gtld-servers.net.	3916	IN	A	192.54.112.30
i.gtld-servers.net.	16217	IN	A	192.43.172.30
j.gtld-servers.net.	39478	IN	A	192.48.79.30
m.gtld-servers.net.	56495	IN	A	192.55.83.30

;; Query time: 60 msec
;; SERVER: 192.168.3.2#53(192.168.3.2)
;; WHEN: Sun Dec 18 05:09:53 EET 2016
;; MSG SIZE  rcvd: 411

Home:

; <<>> DiG 9.10.3-P4-Debian <<>> -tmx php.net
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 17019
;; flags: qr rd ra; QUERY: 1, ANSWER: 1, AUTHORITY: 4, ADDITIONAL: 8

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 4096
;; QUESTION SECTION:
;php.net.			IN	MX

;; ANSWER SECTION:
php.net.		30	IN	MX	0 php-smtp2.php.net.

;; AUTHORITY SECTION:
php.net.		157084	IN	NS	dns4.easydns.info.
php.net.		157084	IN	NS	dns1.easydns.com.
php.net.		157084	IN	NS	dns2.easydns.net.
php.net.		157084	IN	NS	dns3.easydns.org.

;; ADDITIONAL SECTION:
dns1.easydns.com.	165696	IN	A	162.159.24.53
dns1.easydns.com.	159557	IN	AAAA	2400:cb00:2049:1::a29f:1835
dns2.easydns.net.	279	IN	A	198.41.222.254
dns2.easydns.net.	172479	IN	AAAA	2400:cb00:2049:1::c629:defe
dns3.easydns.org.	60	IN	A	64.68.196.10
dns4.easydns.info.	16996	IN	A	194.0.2.19
dns4.easydns.info.	18544	IN	AAAA	2001:678:5::13

;; Query time: 13 msec
;; SERVER: 127.0.0.1#53(127.0.0.1)
;; WHEN: Sun Dec 18 05:13:36 EET 2016
;; MSG SIZE  rcvd: 328

Documentation is actually right, that parameter should be passed by ref, as it's a DNS server that may return something in additional section. As you can see from the code, that array is populated when processing the reply. If wasn't meant to be passed by ref, there would be no reason to populate it inside this function.

The fifth argument is called $raw - it can't be set by ref currently, at least in 5.6, but it's documented to be so. That's what i was talking about.

You're right, the test passed on another machine, without any preparations. I see now in the ticket, that you set an additional nameserver in /etc/resolv.conf. This is not going to work for the arbitrary test environment, so it is should not be done. The test will be skip on Travis as an online test, though. IMO, having some code to skip on unsuitable environment would make sense. Maybe calling dig directly in skipf section, and if it doesn't deliver the required sections, the test should not run. If dig is not available on the system, it should skip as well, as otherwise a false positive could be delivered.

Another thing also - as the $authns arg is touched, it should be tested as well. Probably would make sense to add a separate test for it.

Thanks.
Thanks.

@weltling You're right, the docs are wrong here. Fixed in http://svn.php.net/viewvc?view=revision&revision=341411. The raw argument is also missing from the proto comment in the code.

I think it's okay to merge this fix without a test. Right now get_dns_record() does not have any tests, presumably because of the problems discussed here. It certainly would be good to have tests for this function, but I don't think we should require them for this specific small fix.

@nikic, thanks for the doc fix. Yeah, the patch itself is trivial, i was only giving the comment to the test already supplied. @weirdan, if you're able to improve the test, please do, otherwise please remove the test.

Thanks.

@weltling fixed tests as per your suggestions

Merged with c78fd45. Thanks @weirdan for putting the effort into the QA.