Skip to content

merge hooks for flush icon style and script and use plugin version instead of filemtime #326

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: develop
Choose a base branch
from

Conversation

stklcode
Copy link
Contributor

@stklcode stklcode commented Apr 16, 2025

Fixes and sideshows of #312.

We have a admin_bar_menu hook to add the "flush cache" icon including stylesheet and another hook in the same phase to add the script.
We also determine file modification times of all 3 resources using filemtime() on the CSS/JS file.
And finally, a possible race condition inside the Javascript, if the icon is not rendered before script excution.

  1. Append constant CACHIFY_VERSION instead of dynamic timestamps.
  2. Merge script and style hooks and move them to wp_enqueue_scripts/admin_qneueue_scripts phase.
  3. Rework JS to use `DOMContentLoaded´ event listener.

Resources of the same version should be the same, no matter if the files
are touched or the plugin is re-installed. There are even scenarios
where the modification time is not reliable.

Reduce overhead of accessing the file system and simply add the plugin's
version to scripts and styles.
@stklcode stklcode self-assigned this Apr 16, 2025
stklcode added a commit that referenced this pull request Apr 16, 2025
Resources of the same version should be the same, no matter if the files
are touched or the plugin is re-installed. There are even scenarios
where the modification time is not reliable.

Reduce overhead of accessing the file system and simply add the plugin's
version to scripts and styles.
@stklcode stklcode force-pushed the refactor/resources branch from 4de3a3e to 9379c55 Compare April 16, 2025 16:13
Some users experience issues with the flush icon resources. We currently
register all in the "init" phase, but enqueue in 3 different places.

Merge both style and script into `add_flush_icon_script`, attach it to
`wp_enqueue_scripts` and also add a call from `admin_enqueue_scripts`.
Depending on the actual rendering sequence, users might run into a race
condition where the admin bar element is not rendered at the time of
script execution.

Move the IIFE logic into a DOMContentLoaded event listener and add a
check for the existence of the admin bar element to prevent errors.
Copy link

sonarqubecloud bot commented Apr 18, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
2 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@stklcode stklcode linked an issue Apr 18, 2025 that may be closed by this pull request
@stklcode stklcode added this to the 2.4.2 milestone Apr 18, 2025
@stklcode stklcode added the bug label Apr 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing dependencies shown in Query Monitor
1 participant