Avoiding issue when unable to add domain #885
Conversation
Changed default getDomainExpirationDate value from NULL to MySQL min date to avoid errors when the system in certain situations is unable to lookup the domain expiration date.
Changed more NULL returns to "1000-01-01" i getDomainExpirationDate function.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello & Welcome! :)
Thanks for taking the time to help improve ITFlow. We're excited to review your contributions - we'll review this PR as soon as we can!
Whilst you're waiting, please feel free to check out the forum.
Just so you know, all contributions to ITFlow are licensed under the GNU GPL. By contributing you grant us a perpetual & irrevocable license to include your work in ITFlow.
functions.php
Outdated
| @@ -373,10 +373,11 @@ function encryptLoginEntry($login_password_cleartext) | |||
| // Get domain expiration date | |||
| function getDomainExpirationDate($name) | |||
| { | |||
|
|
|||
| $nullValue = "1000-01-01"; | |||
There was a problem hiding this comment.
This should be 1 January 1970 00:00:00, the start of the current epoch
There was a problem hiding this comment.
I just took mySQL min date, but thats an easy change.
Preferable it should be nullable, but I have no clue of any potential issues in the codebase if the value is null. Just changed that few lines as it was stopping me from moving our docs :D
There was a problem hiding this comment.
I do not know the domain portion of this tool enough to say exactly. I think utc 0 is fine for now.
Changed defualt getDomainExpirationDate value from 1000-01-01 to 1970-01-01.
|
|
Hey @NickyM Thank you for the pull request however this should remain NULL as this will throw off our expire checks that we have throughout the code and will produce some unintended results. Basically the expire date should be NULL if no date is given. |
|
@johnnyq - if it's NULL the server throws a HTTP 500 because the mySQL table isn't nullable. |
|
Hey @NickyM! Thanks for the PR :) Some background: The format for domain whois data varies on each TLD. We use http://lookup.itflow.org:8080/ for domain expiration date parsing. This hosts a Python service, but doesn't seem to cover all TLDs currently. If we add these to #869 we can look into fixing the upstream library at some point. I raised a PR recently as part of this post. -- It looks like we need some better error handling to be honest. It shouldn't give you an error 500, it should just not parse the expiration date and fail gracefully. I'm not sure when this was changed to be "NULL" but given that it's a SQL DATE field that change probably wasn't the best either. Personally, I'm not opposed to allowing NULL values for this field and properly storing the NULL as you've proposed. It's better than refusing to allow the domain altogether. Yes, it might break expiration alerts but that's easy enough to fix as well. |
|
Looking into this further, just running a command-line -- I've raised a separate PR to properly account for null values. Very similar to your PR @NickyM, but has some improved logic and should cover the cron bits off too. If you get a moment, please play around on the PR Review page and see if you can break it? |
We were experiencing HTTP 500 when trying to add the following domains:
The getDomainExpirationDate function returned NULL. The mySQL column isnt nullable.
Returning mySQL min date instead.