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

Fix for Issue #507: Optimize get_formatted_datetime Performance #508

Merged
merged 2 commits into from
Jan 25, 2024

Conversation

deshabhishek007
Copy link
Contributor

This PR provides a solution to the performance issue reported in Issue #507. It introduces caching to the get_formatted_datetime function, reducing database query load and improving efficiency.

  • Implemented caching using WordPress Transients API.
  • Unique cache keys are generated based on function parameters.
  • Transient Expiry time set to 12 Hours

Transient time set to 12 Hours for now.
Copy link
Contributor

@mauteri mauteri left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @deshabhishek007! Couple small clean ups and this will be good to merge. :-)

@@ -399,6 +399,14 @@ protected function get_formatted_datetime(
string $which = 'start',
bool $local = true
): string {

$cache_key = 'formatted_datetime_' . md5($format . $which . ($local ? 'local' : 'gmt'));

Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove this line break and line up = also add more whitespace to meet WordPress standards.

$cache_key   = 'formatted_datetime_' . md5( $format . $which . ( $local ? 'local' : 'gmt' ) );
$cached_date = get_transient( $cache_key );


$cache_key = 'formatted_datetime_' . md5($format . $which . ($local ? 'local' : 'gmt'));

$cached_date = get_transient($cache_key);
Copy link
Contributor

Choose a reason for hiding this comment

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

add line break here.

$cache_key = 'formatted_datetime_' . md5($format . $which . ($local ? 'local' : 'gmt'));

$cached_date = get_transient($cache_key);
if ($cached_date !== false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yoda notation and a bit more whitespace.

if ( false !== $cached_date ) {

@@ -419,6 +427,7 @@ protected function get_formatted_datetime(
$date = wp_date( $format, $ts, $tz );
}

set_transient($cache_key, $date, 43200);
Copy link
Contributor

Choose a reason for hiding this comment

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

line break here before return and add white space

set_transient( $cache_key, $date, HOUR_IN_SECONDS * 12 );

return (string) $date;

- applied WPCS updates
- transient time in easy to read format shared by Mike
@mauteri
Copy link
Contributor

mauteri commented Jan 25, 2024

Looks good, merging! Thanks!

@mauteri mauteri merged commit 5b46b60 into GatherPress:main Jan 25, 2024
10 checks passed
@mauteri
Copy link
Contributor

mauteri commented Jan 27, 2024

@deshabhishek007 Need to revert this change, it is caching and not changing the date when the date changes. We'll need to look at this deeper to ensure this makes fewer DB calls, but does not cache the date so it cannot be changed.

@mauteri
Copy link
Contributor

mauteri commented Jan 27, 2024

@deshabhishek007 I reverted this code here. #516

Steps to reproduce bug I saw is to go to an event, change the date, view the event and the date is the previous date as it is still cached. We need a way to invalidate the cache on save so this doesn't happen. In next attempt, please include some unit tests to ensure this works after caching. Thanks!

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