amphp / dns

Async DNS resolution for PHP based on Amp.
https://amphp.org/dns
MIT License
157 stars 32 forks source link

API Refactoring #36

Closed kelunik closed 8 years ago

kelunik commented 8 years ago

With v0.8.8 we introduced classes as resolvers: https://github.com/amphp/dns/compare/v0.8.7...v0.8.8

We should modify the API to drop the nameserver options from Amp\Dns\query and Amp\Dns\resolve. If anyone needs a different resolver, they should just go ahead and create a new resolver object with the nameserver as argument and execute their query then.

bwoebi commented 8 years ago

I fundamentally disagree with this.

There ought to be one global configuration for all programs, in /etc/resolv.conf for Linux - or the Windows equivalent (TODO: add support for it!).

If you need special nameservers, this is not to be done on a program-level, but either on a host-wide level, or, for specific cases, in the individual query or resolve calls.

The sole point of the classes is overriding the current implementation, not to carry state. State shall be fully transparent to the outside. There may be other caching behavior (e.g. a DNS implementation using shared memory for cache), or a mock for testing a library directly using the DNS. But it shouldn't carry process-wide options.

This also has the big advantage of us not having problems with different parts of the application fighting for different configured nameservers or a similar mess.

kelunik commented 8 years ago

There ought to be one global configuration for all programs, in /etc/resolv.conf for Linux - or the Windows equivalent (TODO: add support for it!).

That one will still be the same.

If you need special nameservers, this is not to be done on a program-level, but either on a host-wide level, or, for specific cases, in the individual query or resolve calls.

Sure, that's why I said:

If anyone needs a different resolver, they should just go ahead and create a new resolver object with the nameserver as argument and execute their query then.

They should use $resolver->query, not Amp\Dns\query which executes with the global resolver.

The sole point of the classes is overriding the current implementation, not to carry state.

Why should it? It's not really about state, but rather about configuration.

Suppose we have a DNS resolver that works by querying an HTTP API instead of using the DNS protocol. That resolver wouldn't be able to use other nameservers.

This also has the big advantage of us not having problems with different parts of the application fighting for different configured nameservers or a similar mess.

That will still be the same. Just the way you're querying alternative nameservers will change from Amp\Dns\resolve with $options["nameserver"] to (new DnsResolver("8.8.8.8"))->query(...).

But it shouldn't carry process-wide options.

They don't. The accessor stores that process-wide state, not the classes.

bwoebi commented 8 years ago

Suppose we have a DNS resolver that works by querying an HTTP API instead of using the DNS protocol. That resolver wouldn't be able to use other nameservers.

This custom DNS resolver would be a standalone entity and not related to our DNS resolver, nor should it implement our interface.

That will still be the same. Just the way you're querying alternative nameservers will change from Amp\Dns\resolve with $options["nameserver"] to (new DnsResolver("8.8.8.8"))->query(...).

They can trivially wrap it in a class { public $nameserver; function query($q, $t) { \Amp\query($q, $t, ["options" => $this->nameserver]); } ... } and similar. That's not the task of this global DNS abstraction.

kelunik commented 8 years ago

Why do we even have these classes then? I see no reason to have the nameserver as an option to query.

bwoebi commented 8 years ago

The only reasons is being able to change internal details (e.g. shared memory DNS caching etc.) or mocking (in tests).

No other reason.

kelunik commented 8 years ago

Ok, I'll ask differently: What's the reason to keep nameserver as an option to Amp\Dns\query? Why shouldn't you just create a new resolver instance with the nameserver passed as argument?

bwoebi commented 8 years ago

a) the other options are also still options. No point in making one option instance specific and the others not?? b) Creating a new instance for every different option is probably only slow and clumsy and you end up with a bunch of objects flying around (given you don't want to re-establish yourself the connection to the server each time etc.) c) keeping options localized to the individual calls keeps code readable (less cognitive overhead), i.e. only use non-defaults where necessary. d) If every instance of the class using DNS is going to instantiate its own DNSResolver (to the same nameserver), that's much more resource intensive [more watchers, more connections, ...].