Skip to content

Bug: Fixes issues for content replacement for the site and media URLs #2014

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

Merged
colinmurphy merged 14 commits into from
Jan 14, 2025
Merged

Bug: Fixes issues for content replacement for the site and media URLs #2014

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
2be8bfe
Fix for #1986. Fixes various issues with content replacement callback…
colinmurphy Dec 24, 2024
70a67ef
Fixed issues with PHPCS
colinmurphy Dec 24, 2024
03a33a9
Updated PR to replace srcset URL for different HTTP Protocols
colinmurphy Dec 30, 2024
be14c9a
Added changeset
colinmurphy Dec 30, 2024
c6ed6d5
Refactoring based off @whoami-pwd feedback. Reduced some of the nes…
colinmurphy Jan 8, 2025
f48551b
Merge branch 'canary' into bug-fix-url-rewrite-issues
colinmurphy Jan 8, 2025
400e7fc
Fixes for linting
colinmurphy Jan 8, 2025
acabb9e
Update plugins/faustwp/includes/replacement/callbacks.php
colinmurphy Jan 9, 2025
8e9a232
Added fixes as per code review from @whoami-pwd
colinmurphy Jan 9, 2025
9770673
Refactoring some of the image srcset replacement.
colinmurphy Jan 10, 2025
71b1d68
Small fix for PHPCS for a missing fullstop
colinmurphy Jan 10, 2025
58d19df
Refactoring some of the media urls with @whoami-pwd
colinmurphy Jan 10, 2025
e37f7b0
Fixes
colinmurphy Jan 10, 2025
11f1db2
Small refactor of parameters with @whoami-pwd
colinmurphy Jan 13, 2025
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
14 changes: 14 additions & 0 deletions .changeset/tidy-toys-bake.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
---
'@faustwp/wordpress-plugin': minor
---

### Fixes

- Fixes various issues with content replacement callback functions and replacing the site_url and media_urls
- Fixed an issue with content replacement when media replacement was disabled and rewrites enabled, it was overwriting and updating the media URL to the frontend URL rather than leaving it as the original site URL


### Added

- Added 6.6 and 6.7 to Github Actions
- Added 2 new filters for `faustwp_get_wp_site_urls` and `faustwp_get_wp_site_media_urls` to allow users add/remove/edit site and media URLS for the content replacement.
2 changes: 1 addition & 1 deletion .github/workflows/unit-test-plugin.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ jobs:
runs-on: ubuntu-22.04
strategy:
matrix:
wordpress: [ '6.5', '6.4', '6.3', '6.2', '6.1' ]
wordpress: [ '6.7', '6.6', '6.5', '6.4', '6.3', '6.2', '6.1' ]
steps:
- name: Checkout
uses: actions/checkout@v4
Expand Down
3 changes: 3 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ process.yml
!.env.local.sample
build/
faustjs.code-workspace
plugins/faustwp/.docker/plugins/akismet/
plugins/faustwp/.docker/plugins/hello.php
plugins/faustwp/.docker/plugins/index.php

# Ignore the WordPress source where used by various development environments
wordpress/
2 changes: 1 addition & 1 deletion plugins/faustwp/docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ services:
links:
- db
environment:
WP_VERSION: ${WP_VERSION:-6.5}
WP_VERSION: ${WP_VERSION:-6.7}
WORDPRESS_DB_HOST: db
WORDPRESS_DB_NAME: wordpress
WORDPRESS_DB_USER: root
Expand Down
116 changes: 69 additions & 47 deletions plugins/faustwp/includes/replacement/callbacks.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,36 +31,44 @@
*
* @return string The post content.
*/
function content_replacement( $content ) {
$use_wp_domain_for_permalinks = ! domain_replacement_enabled();
$use_wp_domain_for_media = use_wp_domain_for_media();
function content_replacement( string $content ): string {

if ( $use_wp_domain_for_permalinks && $use_wp_domain_for_media ) {
$replace_content_urls = domain_replacement_enabled();
$replace_media_urls = ! use_wp_domain_for_media();

if ( ! $replace_content_urls && ! $replace_media_urls ) {
return $content;
}

$replacement = faustwp_get_setting( 'frontend_uri' );
if ( ! $replacement ) {
$replacement = '/';
$wp_site_urls = faustwp_get_wp_site_urls();
if ( empty( $wp_site_urls ) ) {
return $content;
}

$site_url = site_url();
$media_dir = str_replace( $site_url, '', wp_upload_dir()['baseurl'] );
$media_url = $site_url . $media_dir;
$wp_media_urls = faustwp_get_wp_media_urls();
$relative_upload_url = faustwp_get_relative_upload_url( $wp_site_urls );
$frontend_uri = faustwp_get_setting( 'frontend_uri' );
if ( ! $frontend_uri ) {
$frontend_uri = '/';
}

if ( $replace_content_urls && $replace_media_urls ) {
foreach ( $wp_site_urls as $site_url ) {
$content = str_replace( $site_url, $frontend_uri, $content );
}

if ( $use_wp_domain_for_permalinks && ! $use_wp_domain_for_media ) {
$content = str_replace( $media_url, $replacement . $media_dir, $content );
return $content;
}

if ( ! $use_wp_domain_for_permalinks && ! $use_wp_domain_for_media ) {
$content = str_replace( $site_url, $replacement, $content );
return $content;
if ( $replace_media_urls ) {
$wp_media_site_url = $frontend_uri . $relative_upload_url;

return faustwp_replace_media_url( $content, $wp_media_urls, $wp_media_site_url );
}

if ( ! $use_wp_domain_for_permalinks && $use_wp_domain_for_media ) {
$content = preg_replace( "#{$site_url}(?!{$media_dir})#", "{$replacement}", $content );
return $content;
foreach ( $wp_site_urls as $site_url ) {
$pattern_exclude_media_urls = '#' . preg_quote( $site_url, '#' ) . "(?!{$relative_upload_url}(\/|$))#";
Copy link
Member

@whoami-pwd whoami-pwd Jan 9, 2025

Choose a reason for hiding this comment

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

I believe that we can implode the urls into the pattern itself instead of preg_replace it multiple times inside foreach. I guess if the urls array is less then ~50 it is faster and more compact.

Copy link
Member Author

@colinmurphy colinmurphy Jan 9, 2025

Choose a reason for hiding this comment

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

I tried to use implode but the only way I was able to do this is with array map. I don't know if the performance is worth it compared to readibility as I find the array map part not very readable.

	$pattern_exclude_media_urls = '#(' . implode( '|', array_map( function( $url ) {
			return preg_quote( $url, '#' );
		}, $wp_site_urls ) ) . ")(?!{$relative_upload_url}(\/|$))#";

	return preg_replace( $pattern_exclude_media_urls, $frontend_uri, $content );

Happy for a better suggesting on how to do the implode part 👍

Copy link
Member Author

@colinmurphy colinmurphy Jan 9, 2025

Choose a reason for hiding this comment

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

I was actually able to tidy up the array_map based on your suggestion above so thanks 😄

$content = preg_replace( $pattern_exclude_media_urls, $frontend_uri, $content );
}

return $content;
Expand All @@ -86,6 +94,7 @@ function image_source_replacement( $content ) {
"#src=\"{$frontend_uri}/#",
'#src="/#',
);

return preg_replace( $patterns, "src=\"{$site_url}/", $content );
}

Expand All @@ -95,39 +104,44 @@ function image_source_replacement( $content ) {
*
* @param array $sources One or more arrays of source data to include in the 'srcset'.
*
* @return string One or more arrays of source data.
* @return array One or more arrays of source data.
*/
function image_source_srcset_replacement( $sources ) {
$use_wp_domain_for_media = use_wp_domain_for_media();
$frontend_uri = faustwp_get_setting( 'frontend_uri' );
$site_url = site_url();

/**
* For urls with no domain or the frontend domain, replace with the WP site_url.
* This was the default replacement pattern until Faust 1.2, at which point this
* was adjusted to correct replacement bugs.
*/
$patterns = array(
"#^{$site_url}/#",
'#^/#',
);

$replacement = $frontend_uri;
if ( ! is_array( $sources ) || empty( $sources ) ) {
return $sources;
}

/**
* If using WP domain for media and a frontend URL is encountered, rewrite it to WP URL.
*/
if ( $use_wp_domain_for_media ) {
$patterns = array(
"#^{$frontend_uri}/#",
'#^/#',
);
$replacement = $site_url;
$replace_media_urls = ! use_wp_domain_for_media();
$wp_site_urls = faustwp_get_wp_site_urls();
if ( empty( $wp_site_urls ) ) {
return $sources;
}

if ( is_array( $sources ) ) {
foreach ( $sources as $width => $source ) {
$sources[ $width ]['url'] = preg_replace( $patterns, "$replacement/", $source['url'] );
$wp_media_urls = faustwp_get_wp_media_urls();
$relative_upload_url = faustwp_get_relative_upload_url( $wp_site_urls );
$frontend_uri = faustwp_get_setting( 'frontend_uri' );
Copy link
Member

@whoami-pwd whoami-pwd Jan 10, 2025

Choose a reason for hiding this comment

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

Please typecast here to make sure it is string with some default value.

Copy link
Member Author

@colinmurphy colinmurphy Jan 10, 2025

Choose a reason for hiding this comment

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

This is not allowed by our phpcs settings 😅

Copy link
Member Author

@colinmurphy colinmurphy Jan 10, 2025

Choose a reason for hiding this comment

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

Sorry I was wrong here.


$wp_media_site_url = $frontend_uri . $relative_upload_url;
$patterns = array(
"#^{$frontend_uri}/#",
'#^/#',
);

foreach ( $sources as $width => $source ) {
if ( $replace_media_urls ) {
if ( substr( $source['url'], 0, strlen( $relative_upload_url ) ) === $relative_upload_url ) {
$sources[ $width ]['url'] = $frontend_uri . $source['url'];
} else {
$sources[ $width ]['url'] = faustwp_replace_media_url( $source['url'], $wp_media_urls, $wp_media_site_url );
}
} else {
$url = $source['url'];

foreach ( $wp_site_urls as $wp_site_url ) {
$url = preg_replace( $patterns, $wp_site_url . '/', $url );
}
$sources[ $width ]['url'] = $url;
}
}

Expand Down Expand Up @@ -240,7 +254,15 @@ function post_preview_link( $link, $post ) {
*/
function post_link( $link ) {
global $pagenow;
$target_pages = array( 'admin-ajax.php', 'index.php', 'edit.php', 'post.php', 'post-new.php', 'upload.php', 'media-new.php' );
$target_pages = array(
'admin-ajax.php',
'index.php',
'edit.php',
'post.php',
'post-new.php',
'upload.php',
'media-new.php',
);

// phpcs:ignore WordPress.Security.NonceVerification.Missing -- Nonce verified in `is_ajax_generate_permalink_request()` and `is_wp_link_ajax_request()`.
if ( empty( $_POST ) && 'post-new.php' === $pagenow ) {
Expand All @@ -251,7 +273,7 @@ function post_link( $link ) {
if ( in_array( $pagenow, $target_pages, true )
&& is_ajax_generate_permalink_request()
) {
return $link;
return $link;
}

if (
Expand Down
86 changes: 86 additions & 0 deletions plugins/faustwp/includes/replacement/functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,3 +131,89 @@ function is_wp_link_ajax_request(): bool {
&& ! empty( $_POST['action'] )
&& 'wp-link-ajax' === $_POST['action'] );
}

/**
* Get all site URLs for each possible HTTP protocol
*/
function faustwp_get_wp_site_urls() {
Copy link
Member

@whoami-pwd whoami-pwd Jan 9, 2025

Choose a reason for hiding this comment

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

Could you please add the return type here and to the phpdoc?

Copy link
Member Author

@colinmurphy colinmurphy Jan 9, 2025

Choose a reason for hiding this comment

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

Good spot 💯

Copy link
Member Author

@colinmurphy colinmurphy Jan 9, 2025

Choose a reason for hiding this comment

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

Updated 👍


$site_url = site_url();
$host_url = wp_parse_url( $site_url, PHP_URL_HOST );

if ( is_string( $host_url ) ) {
if ( substr( $site_url, 0, 5 ) === 'http:' ) {
$urls = array(
'http://' . $host_url,
'https://' . $host_url,
'//' . $host_url,
);
} else {
$urls = array(
'https://' . $host_url,
'http://' . $host_url,
'//' . $host_url,
);
}
} else {
$urls = array( $site_url );
}

return apply_filters( 'faustwp_get_wp_site_urls', $urls );
}

/**
* Get all media urls based off the available site urls
*
* @return array
*/
function faustwp_get_wp_media_urls() {
Copy link
Member

@whoami-pwd whoami-pwd Jan 9, 2025

Choose a reason for hiding this comment

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

Let's provide some params to the function in order to eliminte some duplications of code.
Also please provide proper array typing in the doc.

$site_urls = faustwp_get_wp_site_urls();
$upload_url = faustwp_get_relative_upload_url( $site_urls );

if ( ! is_string( $upload_url ) ) {
Copy link
Member

@whoami-pwd whoami-pwd Jan 10, 2025

Choose a reason for hiding this comment

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

And then we will not need the extra verification like this, because we will force parameter with string type.

return apply_filters( 'faustwp_get_wp_site_media_urls', array() );
}

$media_urls = array();
foreach ( $site_urls as $site_url ) {
$media_urls[] = $site_url . $upload_url;
}

return apply_filters( 'faustwp_get_wp_site_media_urls', $media_urls );
}


/**
* Gets the relative wp-content upload URL.
*
* @param array<string> $site_urls An array of site URLs.
* @return string The relative upload URL.
*/
function faustwp_get_relative_upload_url( $site_urls ) {
$upload_dir = wp_upload_dir()['baseurl'];
foreach ( $site_urls as $site_url ) {
if ( strpos( $upload_dir, $site_url ) !== false ) {
return (string) str_replace( $site_url, '', $upload_dir );
}
}

return false;
}

/***
* Replaces the media URL for various media urls
*
* @param string $content The content to be updated with the new media URL.
* @param array $wp_media_urls An array of media URLS.
* @param string $replace_url The media URL to be updated to.
*
* @return string
*/
function faustwp_replace_media_url( string $content, array $wp_media_urls, string $replace_url ) {
Copy link
Member

@whoami-pwd whoami-pwd Jan 9, 2025

Choose a reason for hiding this comment

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

I am not sure if this function is needed. It looks like a wrapper to the str_replace with an alternative parameters order.
Please remember that the str_replace can also accept the array as a search param, so no need to loop through each one.

Copy link
Member Author

@colinmurphy colinmurphy Jan 9, 2025

Choose a reason for hiding this comment

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

We use this function in 2 places.Happy to extract the functionality out if you prefer?

Copy link
Member

@whoami-pwd whoami-pwd Jan 10, 2025

Choose a reason for hiding this comment

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

Yeah, my point is that the function itself is just a wrapper for the str_replace. We don't actually do anything except calling the str_replace here.

Copy link
Member Author

@colinmurphy colinmurphy Jan 10, 2025

Choose a reason for hiding this comment

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

Ah sorry I see your point now. I will remove it as a function should really be doing more that one operation


foreach ( $wp_media_urls as $media_url ) {
$content = str_replace( $media_url, $replace_url, $content );
}

return (string) $content;
}
Loading
Loading