pereorga / minimalist-web-notepad

Minimalist Web Notepad
https://notes.orga.cat
1.01k stars 276 forks source link

Delete file when user delete everything in textarea #17

Closed qex closed 6 years ago

qex commented 6 years ago

The main purpose of this PR is to avoid empty files leaving.

Original code will leave a 0 byte file when user clear textarea, now this file will be removed.

Additionally I've made two minor optimizations, change rand() to mt_rand() and pick strlen() out of loop.

Which will both speed up the script running a tiny little bit.

Based on commit 75c1f37, recover the suffix slash of curl to increase accuracy.

I've discovered this project 2 hours ago, and captivated by it immediately.

Thank you for bringing us such an amazing notepad. :yum:

pereorga commented 6 years ago

Hi,

Thank you for your contribution.

The main purpose of this PR is to avoid empty files leaving.

Please do not put different things in a single request, it helps to keep the git history more readable. Otherwise, the review (and merge) is more difficult.

Original code will leave a 0 byte file when user clear textarea, now this file will be removed.

That is a clever idea, thanks! Can you create a separate PR with this? Also, please can you keep the same code standards (brackets, spacing, etc.), i.e.:

// Avoid leaving an empty file, empty() won't work in this case.
if (!strlen($_POST['t'])) {
    unlink($path);
}
else {
    // Update file.
    file_put_contents($path, $_POST['t']);
}

(In the example, I tried to improve the comment too)

Additionally I've made two minor optimizations, change rand() to mt_rand() and pick strlen() out of loop.

I'm happy to merge a separate PR with the mt_rand() optimization.

I wouldn't merge the strlen() optimization because that adds an extra variable for a function rarely executed (1 times max). Also, in my opinion, that change would make the function less readable.

recover the suffix slash of curl to increase accuracy

Are you sure the curl user agent will always end with a slash? By default, yes, but maybe someone could manually specify the curl user agent without a version. And at the same time, I don't think there are other user agents that start with curl and that are not curl.

qex commented 6 years ago

Got it, I'll do that separately.

Every curl user-agent end with a slash that I've seen, however I don't think there's other user-agent start with curl besides curl itself too.

Add that slash probably make the code more strict ?

pereorga commented 6 years ago

Add that slash probably make the code more strict ?

It could, I'm unsure on that one. But the only downside I see here is potentially giving a raw file to a user not using curl (but using a client that starts with curl). I think that if the user agent starts with 'curl', it probably means that the client is curl, a fork of curl, or a client that behaves like curl, and then the raw output should be fine IMO.

qex commented 6 years ago

Maybe there's some browser named "Curlews" or "Curlicue" and doesn't start their user-agent with "Mozilla/"?

Well just kidding, ignore me. :stuck_out_tongue_winking_eye:

pereorga commented 6 years ago

Curlews/232 or Curlicue/13 won't match either, because the capital letter. But that's a valid point :)

FWIW, I just tried to search Google/Bing using 'curl' and 'Curlews' user agents and I get a 403 response in Google, and weird HTML in Bing.