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.

Sign up for GitHub

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

Jump to bottom

Preventing infinite loop #25

Merged
merged 2 commits into from
Oct 14, 2024
Merged

Preventing infinite loop #25

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6218b64
Preventing infinite loop
webmandesign Apr 17, 2024
a9652ca
Removing `is_null()`
webmandesign Apr 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 28 additions & 9 deletions wptt-webfont-loader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* Download webfonts locally.
*
* @package wptt/font-loader
* @link https://github.com/WPTT/webfont-loader
* @license https://opensource.org/licenses/MIT
*/

Expand Down Expand Up @@ -101,9 +102,9 @@ class WPTT_WebFont_Loader {
*
* @access protected
* @since 1.1.0
* @var string
* @var null|string
*/
protected $css;
protected $css = null;

/**
* Cleanup routine frequency.
Expand Down Expand Up @@ -138,6 +139,11 @@ public function __construct( $url = '' ) {
*/
public function get_url() {

// If remote URL is empty just return itself.
if ( empty( $this->remote_url ) ) {
return $this->remote_url;
}

// Check if the local stylesheet exists.
if ( $this->local_file_exists() ) {

Expand Down Expand Up @@ -180,10 +186,20 @@ public function get_local_stylesheet_url() {
*/
public function get_styles() {

// If remote URL is empty, set empty string.
if ( empty( $this->remote_url ) ) {
$this->css = '';
}

// If the CSS is set already, return it.
if ( is_string( $this->css ) ) {
return $this->css;
}

// If we already have the local file, return its contents.
$local_stylesheet_contents = $this->get_local_stylesheet_contents();
if ( $local_stylesheet_contents ) {
return $local_stylesheet_contents;
$this->css = $this->get_local_stylesheet_contents();
if ( ! empty( $this->css ) ) {
return $this->css;
}

// Get the remote URL contents.
Expand Down Expand Up @@ -220,7 +236,6 @@ public function get_styles() {
* @return string|false Returns the remote URL contents.
*/
public function get_local_stylesheet_contents() {
$local_path = $this->get_local_stylesheet_path();

// Check if the local stylesheet exists.
if ( $this->local_file_exists() ) {
Expand All @@ -232,7 +247,7 @@ public function get_local_stylesheet_contents() {
}

ob_start();
include $local_path;
include $this->get_local_stylesheet_path();
return ob_get_clean();
}

Expand Down Expand Up @@ -464,7 +479,7 @@ protected function write_stylesheet() {

// If the folder doesn't exist, create it.
if ( ! file_exists( $this->get_fonts_folder() ) ) {
$this->get_filesystem()->mkdir( $this->get_fonts_folder(), FS_CHMOD_DIR );
$filesystem->mkdir( $this->get_fonts_folder(), FS_CHMOD_DIR );
}

// If the file doesn't exist, create it. Return false if it can not be created.
Expand All @@ -474,7 +489,7 @@ protected function write_stylesheet() {

// If we got this far, we need to write the file.
// Get the CSS.
if ( ! $this->css ) {
if ( is_null( $this->css ) ) {
Copy link

@aristath aristath Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
if ( is_null( $this->css ) ) {
if ( null === $this->css ) {

Copy link
Author

Choose a reason for hiding this comment

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

Hi @aristath Can you please explain the reason for this change?

Copy link

Choose a reason for hiding this comment

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

In WordPress Core we've already replaced all instances of is_null with strict null === $x checks.
The reason behind that change was performance (strict checks are faster than an is_null call), as well as readability and consistency 👍

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for clarification. I've updated the code.

Copy link

Choose a reason for hiding this comment

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

Hi there! I noticed that the changes for the stricter null === $x check haven't been made yet. I believe this would align better with WordPress Core standards and improve performance. Can we proceed with implementing this check? Thanks!

$this->get_styles();
}

Expand Down Expand Up @@ -529,6 +544,10 @@ public function set_font_format( $format = 'woff2' ) {
/**
* Check if the local stylesheet exists.
*
* The name of this method is wrong. Should be "no_local_file_exists()"
* as it returns true if the file does NOT exist.
* Keeping the original name not to break 3rd party scripts.
*
* @access public
* @since 1.1.0
* @return bool
Expand Down