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

Adds Filters to Geocoder; Sets options to autoload=false #217

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

Conversation

bozdoz
Copy link
Owner

@bozdoz bozdoz commented Apr 21, 2023

No description provided.

class.geocoder.php Outdated Show resolved Hide resolved
class.geocoder.php Outdated Show resolved Hide resolved
@bozdoz
Copy link
Owner Author

bozdoz commented Apr 25, 2023

Using these new filters, a consumer can handle the addresses themselves:

<?php

function getit($address_key, $plain_address){
  echo "<pre>". "in getit";
  var_dump($address_key);
  var_dump($plain_address);
  echo "</pre>";
}

add_filter( 'leaflet_geocoder_get_cache', 'getit', 10, 2 );

function setit($key, $value){
  echo "<pre>". "in setit";
  var_dump($key);
  var_dump($value);
  echo "</pre>";
}

add_filter( 'leaflet_geocoder_set_cache', 'setit', 10, 2 );

function update_caches($address){
  echo "<pre>". "in update_caches";
  var_dump($address);
  echo "</pre>";
}

add_filter( 'leaflet_geocoder_update_caches', 'update_caches' );

Screenshot 2023-04-25 at 8 53 15 PM

@bozdoz
Copy link
Owner Author

bozdoz commented Apr 26, 2023

And the full example, using the filters to do md5 hashing, and transients from #218:

<?php

// theme/functions.php

//
// geocoders save to transients
//
const GEOCACHE_PREFIX = 'lm_';

// get transient
function leaflet_transient_get($key, $plain_address){
  $transient_key = GEOCACHE_PREFIX . md5($key);

  return get_transient( $transient_key );
}

add_filter( 'leaflet_geocoder_get_cache', 'leaflet_transient_get', 10, 2 );

// set transient
function leaflet_transient_set($key, $value){
  $transient_key = GEOCACHE_PREFIX . md5($key);
  set_transient( $transient_key, $value, MONTH_IN_SECONDS );
  
  // return false to avoid saving to options table
  return false;
}

add_filter( 'leaflet_geocoder_set_cache', 'leaflet_transient_set', 10, 2 );

// don't update cache key array
function update_caches($address){
  return false;
}

add_filter( 'leaflet_geocoder_update_caches', 'update_caches' );

// custom cleanup function
function leaflet_remove_caches() {
  global $wpdb;
  
  // _transient_ is prefixed to all transient keys in the options table
  $prefix = '_transient_' . GEOCACHE_PREFIX;
  $prefix = $wpdb->esc_like( $prefix );

  $sql    = "SELECT `option_name` FROM $wpdb->options WHERE `option_name` LIKE '%s'";
  $keys   = $wpdb->get_results($wpdb->prepare($sql, $prefix . '%'), ARRAY_A);

  if (is_wp_error($keys) || !is_array($keys)) {
      return;
  }

  $transient_keys = array_map(function ($key) {
      // Remove '_transient_' from the option name.
      return ltrim($key['option_name'], '_transient_');
  }, $keys);

  foreach ($transient_keys as $transient_name) {
      delete_transient($transient_name);
  }
}

add_action( 'leaflet_geocoder_remove_caches', 'leaflet_remove_caches' );

Seems to work great, refreshing shortcode helper page and clearing geocoder cache from settings

Screenshot 2023-04-25 at 9 14 42 PM

@bozdoz bozdoz changed the title moves geocoders to use single transient key Adds Filters to Geocoder; Sets options to autoload=false Apr 26, 2023
@sonarcloud
Copy link

sonarcloud bot commented May 18, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 106 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

$filtered = apply_filters( 'leaflet_geocoder_get_cache', $address_key, $plain_address );

if ($filtered === $address_key) {
return get_transient( $address_key );
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alright, I've gone back to transients

*/
if (apply_filters('leaflet_geocoder_set_cache', $key, $value)) {
// get user-defined expiry (maybe this should be an admin option?)
$expiry = apply_filters('leaflet_geocoder_expiry', null);
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

power users can define their own transient expiries

if ($expiry === null) {
// stagger caches between 200-400 days to prevent all caches expiring on the same day
$stagger = random_int(200, 400);
$expiry = DAY_IN_SECONDS * $stagger;
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by default, we set expiry randomly between 200 and 400 days


// set to 25 year expiry since we never really want it to expire
// but omitting expiry causes it to autoload
set_transient( self::$locations_key, $locations, YEAR_IN_SECONDS * 25 );
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the transient which holds all address keys is 25 years to avoid expiry and avoid autoload

@@ -104,13 +104,34 @@ public static function init() {
* Leaflet_Map Constructor
*/
private function __construct() {
$this->run_migrations();
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

run migrations before any other initialization of the plugin

return;
}

include_once LEAFLET_MAP__PLUGIN_DIR . 'class.migrations.php';
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only include migrations if there's a version mismatch in the db

wp_register_style('leaflet_stylesheet', $css_url, Array(), null, false);
wp_register_script('leaflet_js', $js_url, Array(), null, true);
wp_register_style('leaflet_stylesheet', $css_url, Array(), LEAFLET_MAP__PLUGIN_VERSION, false);
wp_register_script('leaflet_js', $js_url, Array(), LEAFLET_MAP__PLUGIN_VERSION, true);
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adds plugin version like all the other registered scripts; #222

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I don't think this is right. Adding the plugin version rather than the Leaflet JS version means that if the Leaflet JS version is changed then the cache potentially wouldn't know (if the URL was the same). This is why I would opt for apply_filters('leaflet_js_version', Leaflet_Map::$leaflet_version) instead because it would also allow this to be filtered when the version is changed programatically (as we do).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Furthermore, $in_footer is set to true here but line 218 is set to false. I think that both need to be true or both need to be false which is why the JavaScript is breaking when using "Merge + Minify + Refresh" plugin.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I don't think this is right. Adding the plugin version rather than the Leaflet JS version means that if the Leaflet JS version is changed then the cache potentially wouldn't know (if the URL was the same). This is why I would opt for apply_filters('leaflet_js_version', Leaflet_Map::$leaflet_version) instead because it would also allow this to be filtered when the version is changed programatically (as we do).

This makes me think "null" is better here.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Furthermore, $in_footer is set to true here but line 218 is set to false. I think that both need to be true or both need to be false which is why the JavaScript is breaking when using "Merge + Minify + Refresh" plugin.

Hesitant to change this because we've had issues in the past with people running async and deferred scripts.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes no odds to me. I've developed around it by directly filtering the global $wp_styles and $wp_scripts. Here is what Wordpress says though (script version not plugin version) https://developer.wordpress.org/reference/functions/wp_register_script/#parameters

$ver string|bool|null Optional
String specifying script version number, if it has one, which is added to the URL as a query string for cache busting purposes. If version is set to false, a version number is automatically added equal to current installed WordPress version.

But "null" will definitely not help the cache at all.

Again, it doesn't bother me if you change the scripts to both be in the header or footer because I've developed around it by directly filtering the global $wp_scripts. We chose to put them both in the footer FYI.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UPDATE: We have changed our position on this and moved both scripts to be loaded in the header instead of the footer. This is because loading them in the footer breaks compatibility with Leaflet.GestureHandling https://elmarquis.github.io/Leaflet.GestureHandling/

@@ -204,7 +225,8 @@ public static function enqueue_and_register()
// optional ajax geojson plugin
wp_register_script('tmcw_togeojson', $settings->get('togeojson_url'), Array('jquery'), LEAFLET_MAP__PLUGIN_VERSION, false);

if (defined('WP_DEBUG') && WP_DEBUG) {
// @since 3.4.0
if (defined('LEAFLET_MAP__DEBUG') && LEAFLET_MAP__DEBUG) {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as per #222

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

if (version_compare($db_version, '3.4.0', '<')) {
include_once LEAFLET_MAP__MIGRATION_DIR . '001-v3.4.0-geocoder-transients.php';

migration001();
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

included above; needs to be a unique migration function name; assuming this is ok; maybe I could use "leaflet_map__" prefix?

migration001();

// update version, if for some reason something fails afterwards, and we want to prevent this migration
update_option(LEAFLET_MAP__DB_VERSION_KEY, '3.4.0');
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assuming there may be more migrations in the future, we update db asap so this particular migration never refires

@bozdoz
Copy link
Owner Author

bozdoz commented May 18, 2023

Screenshots of the migration (cycling between master branch and this branch):

v3.3.0

Screenshot 2023-05-17 at 11 54 27 PM

After migration to v3.4.0 (simply by refreshing the browser)

Screenshot 2023-05-17 at 11 55 04 PM

@sonarcloud
Copy link

sonarcloud bot commented Sep 20, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 69 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

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