jeremykendall / php-domain-parser

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

Improve the update-psl script #249

Closed nyamsprod closed 4 years ago

nyamsprod commented 5 years ago

summary

The current update-psl script is just an update version of the script from v3. when #248 was submitted for review we came to the conclusion that the script needed an overall improvement to meet it's primary goal making it easy to update the local cache of the library

proposal

The new system will keep the bin/update-psl entry point and its current use but will add arguments and flags to better control how the script interact with the library cache.

after implementation the following command will be accessible to the end user:

$ php bin/update-psl --rzd --rzd-url=http://localhost/rzd-mirror/list.txt --ttl="8 HOURS" --cache-dir="/tmp"

This lines means:

overall implementation

The following arguments will be added:

The modification will be made so that the current behaviour stays untouched meaning:

$ php bin/update-psl 

will update both cache system using the default values.

nyamsprod commented 5 years ago

@Shardj do you consider modifying the current output content a BC break or not ?

Shardj commented 5 years ago

As in the error messages? No not at all, change them. I don't like the emojis in the current one anyway.

By the way, since there's a shebang for php in the update-psl script you can run it with just bin/update-psl you don't need to have php in front.

nyamsprod commented 5 years ago

Ok I'll change the texts too then... Did I miss any other arguments you may want to add 🤔 or does it meet all your requirements? The PR is working but I need to still fine tune it if you could test it for UX that would be great.

Shardj commented 5 years ago

Being able to specify cache expiry would be helpful too I suppose. I'll happily test it, for me though I'm no longer bothering to force updates as instead I just have cache expiry set to only a day so it's regularly checked.

nyamsprod commented 4 years ago

the new installer is merged in the develop branch and will be release with the next minor version