jeremykendall / php-domain-parser

Public Suffix List based domain parsing implemented in PHP
MIT License
1.16k stars 128 forks source link

#249 improving the update-psl script #250

Closed nyamsprod closed 4 years ago

nyamsprod commented 5 years ago

This PR is a implementation to solve #249

nyamsprod commented 5 years ago

@Shardj the text has been changed ... the PR is ready for review 👍

Shardj commented 5 years ago

I haven't tested it, just had a skim through

nyamsprod commented 5 years ago

How come the curl extension is being checked in the update-psl file instead of installer refresh?

Because the installer requires a Manager instance which relies on interface the CurlHttpClient is just a implementation of the HttpClient.

In updateLocalCache you fwrite to STDERR instead of using Event's iointerface

That's because $event could be null to avoid BC break I should probably throw an exception instead

On line 91 of Installer what's up with the checks against false then making them true?

again to avoid bc break if those arguments are not present we're defaulting into updating all data which is the current behaviour

Shouldn't refresh and execute return bools rather than a ints, those using refresh directly won't want the error code response that a bash interface expects

Installer, 113, should there be a breakline between docblock annotations?

I prefer to separate params/throws/return in docblock this way I can easily what is going on while reading the docblock.

Logger created in updateLocalCache should probably use events iointerface to output

A previous iteration used to do that but then everything is logged and you get no output at all on cli 😢

yes I can change that, no problem.

Installer, 97, 100, 103, may as well directly return values instead of passing them to $retVal which returns them

yes I can change that, no problem.

Shardj commented 5 years ago

You could do a check, if event is null then use fwrite, otherwise use events iointerface, or both. Same as the old functionality, except that had a mock class for if event was missing. Either way, doesn't matter too much, whatever you think is best.