Only add/remove from htaccess file when necessary

[raamdev]

See wpsharks/comet-cache#641

---

Quoting @raamdev from wpsharks/comet-cache#641 (comment):

After some research, I discovered a few issues with the htaccess utilities in ZenCache Pro v151220:

• Every time the plugin options are saved when ZenCache is enabled, ZenCache calls addWpHtaccess(), regardless of whether or not it actually needs to (i.e., whether or not any options are enabled that require adding something to the htaccess file).
• ZenCache does not intelligently keep track of which options are currently enabled that require htaccess rules, and as a result more code than necessary is being run each time the site owner saves the plugin options and addWpHtaccess() is called.

As a result of all this unnecessary work, I believe we're increasing the chances that the site owner might see an error message when the error message isn't even applicable (i.e., ZenCache has no reason to add/remove anything from the htaccess file).

I've submitted a PR (#204) that resolves these issues.

[raamdev]

@jaswsinc A review here when you get a chance. 😄 Thanks!

[jaswrks]

@raamdev I reviewed the code for these changes.

On this line I think the call to findHtaccessMarker() is redundant, since that is the responsibility of removeWpHtaccess() already.

I also think that this block should always be run first, before we check if there is the need to add anything; i.e., remove what was there before we consider adding anything new.

Otherwise it looks good to me.

---

Other Thoughts

I'm a fan of optimization, and I like where you are going with this. However, in this particular case my feeling is that needHtaccessRules() is unnecessary. The addWpHtaccess() method already has this job to do; i.e., as it cycles through each of the template blocks to see if they apply.

So while I don't see anything wrong with needHtaccessRules(), my feeling is that it will add more complexity than is necessary. There are likely to be more and more use cases for minor .htaccess tweaks as we move forward, so having two different routines to work out the logic is going to make it overly complex in my view.

In this line we already bail if nothing needs to be written.

Also, this only runs whenever you update your ZC options, right? So it's not an area of the codebase where optimization is really important.

[raamdev]

On this line I think the call to findHtaccessMarker() is redundant, since that is the responsibility of removeWpHtaccess() already.

I also think that this block should always be run first, before we check if there is the need to add anything; i.e., remove what was there before we consider adding anything new.

I disagree, and here's why:

As of now, if a site owner does not have CDN Filters enabled there is no reason whatsoever to read or write to the .htaccess file.

And yet, as of now, here's what we do every time the site owner saves the plugin options even when there's nothing for us to do with the .htaccess file:

• Call removeWpHtaccess(), which then reads the .htaccess file to look for the unique marker, and then reads the .htaccess file and acquires an exclusive lock, and then searches the .htaccess file for our comment markers and removes them if found and then writes to the .htaccess file and releases the lock.
• Call readHtaccessFile(), which then reads the .htaccess file and acquires an exclusive lock, and then assuming nothing needs to be added to the .htaccess file, calls closeHtaccessFile() which releases the lock.

That means we read the .htaccess file three (3) times, acquire two (2) separate exclusive locks, and write to the .htaccess file once (1). All of that work when we can know ahead of time that we don't need to touch the .htaccess file.

That was my reason for adding needHtaccessRules().

I have seen a handful of reports (including one from Pat) where there was an error message displayed on the Dashboard related to being unable to read/write to the .htaccess file, in a scenario where there was no reason to do so in the first place (i.e., CDN Filters were not enabled). My feeling is that because of all the extra unnecessary work we're doing each time the site owner saves the plugin options, we're occasionally tripping things up and something along the line fails and produces the error message (which goes away on the next refresh because it works fine the next time you try it).

[jaswrks]

All of that work when we can know ahead of time that we don't need to touch the .htaccess file.

That's a good point and I agree.

I have seen a handful of reports (including one from Pat) where there was an error message displayed on the Dashboard related to being unable to read/write to the .htaccess file

At some point it might be a good idea to add some more ways for this to wrong; i.e., increase the number of detailed exceptions that we throw until we can pinpoint more specifically what the problem is.

[raamdev]

@jaswsinc I added a few TODOs in f887948 so that we can revisit improving error reporting detail in the Htaccess Utils; I agree we need to do better there.