Skip to content

Refactored code to ease future development #1

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Patabugen
Copy link

Hey Diego,

I hope you don't mind the unsolicited contributions. I like this script and would like to help expand on it - specially I'd like to make it more generic so it can work with more than just the two hard-coded scripts.

This first pull request is just tidying up the code a bit, the additions I'd like to make will make the code more complicated so I wanted to start from a good grounding.

I've refactored the code into single object instead of multiple loose functions and tweaked it to meet the WordPress Coding Standards.

Refactored code into single object instead of multiple loose functions and refactored code to WordPress Coding Standards
I've moved all the Script Data to a single location (currently hard-coded inside get_scripts_to_cache()), so the next step will be to enable customising what that returns.

I also added the last-cached time to the status screen.
I forgot WordPress likes to support ancient PHP so I've corrected the short Array syntax. I've also added a filter to customise the updated scripts, made 'basename' be calculated (which is possible groundwork for version-based-filename) and added an example to the readme.txt
@lazmo88
Copy link

lazmo88 commented Dec 18, 2017

@Patabugen Love the idea! I was looking into a solution where all (selected) external scripts / css would be saved locally to my own server and updated on given intervals. Very promising PR and suggestion! I guess this would require rewriting the resource links while rendering the page to point to the local versions in order to be compatible with various themes.

As an idea, maybe it would be possible to scan the current theme for external resources and have simple checkbox next to each resource in the settings to enable local cache?

@Patabugen
Copy link
Author

@lazmo88 I did actually spend a lot of time trying to make this plugin more generic too (and spoke with the plugin authour a bit too). Sadly I don't have time to continue working on it but I did discover lots of very complicated bits. See my notes below:

Caching external scripts in a generic way is really hard because:

  • Some scripts have custom get parameters or anchors which need to be preserved
  • Some scripts have language selectors in the URLs
  • Some replacements are not simple replacements (as is the case for one of your Google scripts anyway).

Take a look at these two nasties for example:

I'm wondering if we should do the following:

Have an Options table with ~ 4 columns:
Replacement Place:

  • HTML - Makes replacements in using output buffering
  • wp_enqueue_script - Matches URLs queued with wp_enqueue_script
  • external_script - Makes replacements in external scripts as we cache them. To let us cache scripts added by the third party scripts. This may be a risky business so might be best left alone.

Replacement Type:

  • preg_replace
  • preg_match (for wp_enqueue_script only)
  • str_replace
  • Replacement Pattern - the pattern to search for
  • Replacement String - what to replace it with - including placeholders for preg_replace

and have the user manually fill in all the options.

We can then have a button which loads the homepage (and perhaps others) with a special GET parameter which can search for an collect scripts and add them to the table. This would let the user easily decide not to cache some scripts too.

@lazmo88
Copy link

lazmo88 commented Dec 26, 2017

@Patabugen I did some research regarding the js and css files loaded by WordPress. Most of the files, if not all of them are passed via built-in wp hooks. Here is something similar where they are automatically generating a list of resources to be cached for offline use: https://www.smashingmagazine.com/2017/10/service-worker-single-page-application-wordpress-sites/

I am not sure if that helps, however as far as I understood the wp hooks handles the custom params fine and is quite solid way of listing resources.

Regarding the options panel, I would see simple button that generates a list of resources loaded from external sources and each row / resource would have a checkbox if it should be saved in local server or served directly from external server. You could also specify "default setting" if the script is running periodically via cronjob and new resources are discovered. Cronjob would be also useful for updating the resources every x hours.

To make sure that also resources that are not passed via wp hooks, manual entries could be useful fallback so also these could be controlled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants