Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Static CDN Filters should detect when Permalink Structure contains `.htm` or `.html` #495

Closed
raamdev opened this issue Jun 9, 2015 · 10 comments

Comments

@raamdev
Copy link
Contributor

@raamdev raamdev commented Jun 9, 2015

If a site has a Custom Permalink structure of, say /%year%/%monthnum%/%postname%.html, and that site has enabled the ZenCache Static CDN Filters, then ZenCache will consider any links in the HTML that end with .html (or .htm) as static resources and rewrite the URLs with the configured CDN hostname.

That means that a link to a blog post http://www.example.com/2015/04/welcome-home.html would become something like http://d2at82rz6jurc6.cloudfront.net/2015/04/welcome-home.html, which is incorrect (we should only be rewriting URLs for static content, not for entire blog posts).

What should happen is ZenCache should detect when the Permalink Structure contains .htm or .html and then add the necessary extension to the Static CDN Filters Blacklisted File Extensions.

Temporary Workaround

You can add htm,html to the Static CDN Filters Blacklisted File Extensions. That will prevent ZenCache from rewriting the URLs for the post links.

@raamdev raamdev added this to the Future Release (Pro) milestone Jul 27, 2015
@jaswrks
Copy link

@jaswrks jaswrks commented Aug 21, 2015

Next Actions

Alter Default Whitelisted Extensions

  • Update this class member in the CdnFilters class to the following:

    /**
    * Default whitelisted extensions.
    *
    * @since 150314 Take extensions from WP.
    *
    * @return array Default whitelisted extensions.
    */
    public static function defaultWhitelistedExtensions()
    {
        $extensions = array_keys(wp_get_mime_types());
        $extensions = array_map('strtolower', $extensions);
        $extensions = array_merge($extensions, array('eot', 'ttf', 'otf', 'woff'));
    
        if (($permalink_structure = get_option('permalink_structure'))) {
            if (strcasecmp(substr($permalink_structure, -5), '.html') === 0) {
                $extensions = array_diff($extensions, array('html'));
            } elseif (strcasecmp(substr($permalink_structure, -4), '.htm') === 0) {
                $extensions = array_diff($extensions, array('htm'));
            }
        }
        return array_unique($extensions);
    }
@raamdev raamdev self-assigned this Aug 21, 2015
raamdev added a commit to wpsharks/comet-cache-pro that referenced this issue Sep 18, 2015
- Excludes htm and html when permalink structure ends in .htm or .html

See wpsharks/comet-cache#495
@raamdev
Copy link
Contributor Author

@raamdev raamdev commented Sep 19, 2015

Next Pro Release Changelog:

  • Bug Fix: Fixed a bug with the Static CDN Filters that affected sites using a permalink structure that ended with .htm or .html. Generally, files that end in .htm or .html are considered static files, hence the reason ZenCache was rewriting URLs with those extensions to point at the configured CDN. However, if a site uses .htm or .html in their permalink structure, all links to Posts/Pages within the site will appear to be static files when they are in fact dynamic and therefore should not be rewritten. ZenCache now checks the permalink structure and excludes .htm and .html from the allowed extensions when the permalink structure is using one of these. Props @jaswsinc. See Issue #495.
@raamdev raamdev closed this Sep 19, 2015
@raamdev
Copy link
Contributor Author

@raamdev raamdev commented Oct 2, 2015

ZenCache Pro v151002 has been released and includes changes from this GitHub Issue.

See the ZenCache Pro v151002 release announcement for further details.


This issue will now be locked to further updates. If you have something to add related to this GitHub Issue, please open a new GitHub Issue and reference this one (#495).

@wpsharks wpsharks locked and limited conversation to collaborators Oct 2, 2015
@raamdev raamdev modified the milestones: Next Release (Pro), v151002 (Pro) Nov 28, 2015
@raamdev
Copy link
Contributor Author

@raamdev raamdev commented Nov 28, 2015

@jaswsinc It appears that somewhere between v151002 and v151114, this bug was reintroduced. We received a report (see internal ticket) and I just tested Static CDN Filters with the following Permalink settings to confirm that ZenCache is rewriting all URLs that end with .htm (i.e., any links to posts/pages with the permalink ending in .htm is getting rewritten):

2015-11-28_12-49-23
2015-11-28_12-51-11

@raamdev raamdev reopened this Nov 28, 2015
@wpsharks wpsharks unlocked this conversation Nov 28, 2015
@raamdev
Copy link
Contributor Author

@raamdev raamdev commented Nov 28, 2015

@jaswsinc I was just reviewing the code for this and this looks to me like the bug might be with the assumption that wp_get_mime_types() returns an array that contains two separate keys for htm and html.

Looking at wp_get_mime_types(), I see that the array key is actually htm|html (i.e., it's a single key that contains both values), which means this line in ZenCache will never remove htm|html from $extensions:

$extensions = array_diff($extensions, array('html'));

Finally, what if someone changes their Permalink structure to include .htm after installing ZenCache? I'm not seeing anywhere that we hook into changes to the Permalink structure to check if the Permalinks contain .htm or .html.

@jaswrks
Copy link

@jaswrks jaswrks commented Nov 28, 2015

Oh, good catch. I see this was working properly, but now it is broken for some of those with more than one extension. Referencing: https://github.com/websharks/zencache-pro/blob/150821/src/includes/classes/CdnFilters.php#L544

That's my bad. I left that line out of the outline I posted in this issue prior.

Finally, what if someone changes their Permalink structure to include .htm after installing ZenCache?

The CDN Filter class runs as a part of the plugin (i.e., not in the early phase) but like any other plugin. So it will pick up changes right away.

However, it would be great if we had some hooks that would reset the cache whenever a form is posted from certain menu pages in the Dashboard. For example, if you update your Permalink settings that could trigger a wipe of the cache. Same if you update your general options. Those are things that occur rarely on a live site, but maybe we can think about making ZenCache smarter.

@raamdev
Copy link
Contributor Author

@raamdev raamdev commented Nov 30, 2015

it would be great if we had some hooks that would reset the cache whenever a form is posted from certain menu pages in the Dashboard. For example, if you update your Permalink settings that could trigger a wipe of the cache

This already occurs:

2015-11-30_13-10-17

@jaswrks
Copy link

@jaswrks jaswrks commented Nov 30, 2015

This already occurs

aha! Great! I totally forgot that you added this.

@raamdev
Copy link
Contributor Author

@raamdev raamdev commented Dec 1, 2015

Next Pro Release Changelog:

  • Bug Fix: Fixed a bug with the Static CDN Filters that affected sites using a permalink structure that ended with .htm or .html. This bug was supposed to be fixed back in v151002 but another bug prevented the fix from working properly. This release fixes the issue once and for all. See Issue #495.
@raamdev raamdev closed this Dec 1, 2015
@wpsharks wpsharks locked and limited conversation to collaborators Dec 21, 2015
@raamdev
Copy link
Contributor Author

@raamdev raamdev commented Dec 21, 2015

ZenCache Pro v151220 has been released and includes changes worked on as part of this GitHub Issue. See the release announcement for further details.


This issue will now be locked to further updates. If you have something to add related to this GitHub Issue, please open a new GitHub Issue and reference this one (#495).

@raamdev raamdev removed their assignment Apr 28, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants