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

colinmurphy · 2024-12-24T14:31:05Z

See #1956

This fixes various issues for content replacement callback functions for both the site and media URLs. There is 2 main fixes in this branch.

Fixed issues with site URL and content replacment and not taking into account different HTTP protocols for the site URL (http, https)
Fixed an issue when the configuration setting " Enable Post and Category URL rewrites" was enabled and also "Use the WordPress domain for media URLs in post content" was enabled, it was updating the media URLs to the frontend URL even though it should of been the WP site URL

The branch also adds 2 new filters to allow developers to modify the URLS used for content replacement

faustwp_get_wp_site_urls - An array of site URLs
faustwp_get_wp_site_media_urls - An array of media site URLs

This branch also updated Github actions to also use WordPress 6.6. and 6.7

Tasks

I have signed a Contributor License Agreement (CLA) with WP Engine.
If a code change, I have written testing instructions that the whole team & outside contributors can understand.
I have written and included a comprehensive changeset to properly document the changes I've made.

Description

See above.

Related Issue(s):

#1956

Testing

Setup

As per issue #1956 I setup a WordPress Multisite (sub-domains) with the latest version of 6.7.1 using Local.

I setup 3 sub-domains and installed FaustWP, WPGraphQL & ACF and one of the sites was setup as http://alpha.multisite.local/

I setup a field group for pages and I setup a WYSIWG editor field with the field name summary

I then updated the sample page to have an image for the WYSIWG field and also a link e.g.

<img class="alignnone size-medium wp-image-10" src="http://alpha.multisite.local/wp-content/uploads/sites/2/2024/12/WP-Engine-Blue-888x459-1-300x155.webp" alt="" width="300" height="155" />

<a href="https://alpha.multisite.local/index.php/hello-world/">Hello World</a>

I then used the following GraphQL query in the WPGraphQL IDE for debugging and fixing the issues

{
  pages {
    nodes {
      pageFields {
        summary
      }
    }
  }
}

I then access the site on HTTPS rather than HTTP (site_url)

Tests

Note http://localhost:3000 is the frontend URL set in Faust WP and I am accessing the site on HTTPS e.g. https://alpha.multisite.local/wp-admin/

Test 1: URL Rewrites Enabled

Enable Post and Category URL rewrites = enabled
Use the WordPress domain for media URLs in post content = disabled

Query

{
  pages {
    nodes {
      pageFields {
        summary
      }
    }
  }
}

Output

{
  "data": {
    "pages": {
      "nodes": [
        {
          "pageFields": {
            "summary": "<p><img loading=\"lazy\" decoding=\"async\" class=\"alignnone size-medium wp-image-10\" src=\"http://localhost:3000/wp-content/uploads/sites/2/2024/12/WP-Engine-Blue-888x459-1-300x155.webp\" alt=\"\" width=\"300\" height=\"155\" srcset=\"http://localhost:3000/wp-content/uploads/sites/2/2024/12/WP-Engine-Blue-888x459-1-300x155.webp 300w, http://localhost:3000/wp-content/uploads/sites/2/2024/12/WP-Engine-Blue-888x459-1-768x397.webp 768w, http://localhost:3000/wp-content/uploads/sites/2/2024/12/WP-Engine-Blue-888x459-1.webp 888w\" sizes=\"auto, (max-width: 300px) 100vw, 300px\" /></p>\n<p><a href=\"http://localhost:3000/index.php/hello-world/\">Hello World</a></p>\n"
          }
        }
      ]
    }
  }
}

All 3 URLs (image src, srcset and link) were updated correctly to the frontend URL http://localhost:3000/

Test 2: URL Rewrites Enabled and Media Rewrites Disabled

Enable Post and Category URL rewrites = enabled
Use the WordPress domain for media URLs in post content = enabled

Query

{
  pages {
    nodes {
      pageFields {
        summary
      }
    }
  }
}

Output

{
  "data": {
    "pages": {
      "nodes": [
        {
          "pageFields": {
            "summary": "<p><img loading=\"lazy\" decoding=\"async\" class=\"alignnone size-medium wp-image-10\" src=\"http://alpha.multisite.local/wp-content/uploads/sites/2/2024/12/WP-Engine-Blue-888x459-1-300x155.webp\" alt=\"\" width=\"300\" height=\"155\" srcset=\"https://alpha.multisite.local/wp-content/uploads/sites/2/2024/12/WP-Engine-Blue-888x459-1-300x155.webp 300w, https://alpha.multisite.local/wp-content/uploads/sites/2/2024/12/WP-Engine-Blue-888x459-1-768x397.webp 768w, https://alpha.multisite.local/wp-content/uploads/sites/2/2024/12/WP-Engine-Blue-888x459-1.webp 888w\" sizes=\"auto, (max-width: 300px) 100vw, 300px\" /></p>\n<p><a href=\"http://localhost:3000/index.php/hello-world/\">Hello World</a></p>\n"
          }
        }
      ]
    }
  }
}

The image URL wasn't updated and the link was updated to the frontend URL.

Test 3: URL Rewrites Disabled and Media Rewrites Disabled

Enable Post and Category URL rewrites = disabled
Use the WordPress domain for media URLs in post content = enabled

Query

{
  pages {
    nodes {
      pageFields {
        summary
      }
    }
  }
}

Output

{
  "data": {
    "pages": {
      "nodes": [
        {
          "pageFields": {
            "summary": "<p><img loading=\"lazy\" decoding=\"async\" class=\"alignnone size-medium wp-image-10\" src=\"http://alpha.multisite.local/wp-content/uploads/sites/2/2024/12/WP-Engine-Blue-888x459-1-300x155.webp\" alt=\"\" width=\"300\" height=\"155\" srcset=\"https://alpha.multisite.local/wp-content/uploads/sites/2/2024/12/WP-Engine-Blue-888x459-1-300x155.webp 300w, https://alpha.multisite.local/wp-content/uploads/sites/2/2024/12/WP-Engine-Blue-888x459-1-768x397.webp 768w, https://alpha.multisite.local/wp-content/uploads/sites/2/2024/12/WP-Engine-Blue-888x459-1.webp 888w\" sizes=\"auto, (max-width: 300px) 100vw, 300px\" /></p>\n<p><a href=\"https://alpha.multisite.local/index.php/hello-world/\">Hello World</a></p>\n"
          }
        }
      ]
    }
  }
}

Neither the image src, srcset or link was updated to use the frontend URL

Test 4: URL Rewrites Disabled and Media Rewrites Enabled

Enable Post and Category URL rewrites = disabled
Use the WordPress domain for media URLs in post content = disabled

Query

{
  pages {
    nodes {
      pageFields {
        summary
      }
    }
  }
}

Output

{
  "data": {
    "pages": {
      "nodes": [
        {
          "pageFields": {
            "summary": "<p><img loading=\"lazy\" decoding=\"async\" class=\"alignnone size-medium wp-image-10\" src=\"http://localhost:3000/wp-content/uploads/sites/2/2024/12/WP-Engine-Blue-888x459-1-300x155.webp\" alt=\"\" width=\"300\" height=\"155\" srcset=\"http://localhost:3000/wp-content/uploads/sites/2/2024/12/WP-Engine-Blue-888x459-1-300x155.webp 300w, http://localhost:3000/wp-content/uploads/sites/2/2024/12/WP-Engine-Blue-888x459-1-768x397.webp 768w, http://localhost:3000/wp-content/uploads/sites/2/2024/12/WP-Engine-Blue-888x459-1.webp 888w\" sizes=\"auto, (max-width: 300px) 100vw, 300px\" /></p>\n<p><a href=\"https://alpha.multisite.local/index.php/hello-world/\">Hello World</a></p>\n"
          }
        }
      ]
    }
  }
}

Both the image src and srcset URLs were updated with the frontend URL where the link stayed the same

Screenshots

Documentation Changes

There was 2 new filters added

faustwp_get_wp_site_urls for allowing developers to add or modify an array of site URLS used for content replacement
faustwp_get_wp_site_media_urls for allowign developers to add or modify an array of media site URLS for content replacemnt

Dependant PRs

… and also updates tests to use WP 6.6 and 6.7 Fixes various issues with using content replacement for the site URL.

changeset-bot · 2024-12-24T14:31:08Z

🦋 Changeset detected

Latest commit: e37f7b0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@faustwp/wordpress-plugin	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2024-12-24T14:34:11Z

📦 Next.js Bundle Analysis for @faustwp/getting-started-example

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

colinmurphy · 2024-12-24T14:36:04Z

@josephfusco This is the draft PR for the site URL issues. This fixes the various issues as per our call last week but I also want to update the image srcset replacement with the same functionality so that we replace all HTTP protocols e.g. HTTP, HTTPS or //

colinmurphy · 2024-12-24T16:46:22Z

Just noting I am working on also updating the image_source_srcset_replacement function callback aswell to use all HTTP protocols so I will have an updated PR next week after the holidays.

theodesp · 2024-12-30T15:47:25Z

Awesome!

colinmurphy · 2024-12-30T15:59:06Z

Awesome!

Thanks @theodesp I have also now updated the PR with details on how to replicate the issue and test the issue.

josephfusco

Looks good to me!

…ng for cotent replacement.

colinmurphy · 2025-01-08T19:19:17Z

@josephfusco @whoami-pwd

Can you give this another review please. Thanks @whoami-pwd for the suggestions around optimizing some the loops on our call earlier.

I have tested locally again the different config for URLs and it is all working as expected but if you could test aswell that would be great please👍

whoami-pwd

Great work on addressing these issues! Please check the comments for a few suggestions.

whoami-pwd · 2025-01-09T15:13:14Z

plugins/faustwp/includes/replacement/functions.php

+ *
+ * @return string The replaced string
+ */
+function faustwp_replace_media_url( string $content, array $wp_media_urls, string $replace_url ) {


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.

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

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.

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

whoami-pwd · 2025-01-09T15:14:13Z

plugins/faustwp/includes/replacement/functions.php

+ *
+ * @return mixed The replaced content
+ */
+function faustwp_replace_urls( array $patterns, mixed $wp_site_urls, mixed $content ) {


Maybe it is better for us to keep things more simple here. Let's try to avoid using that function.

Thanks @whoami-pwd

I was hoping to re-use that function when I built it, but it is only used once so I will remove it 👍

whoami-pwd · 2025-01-09T15:16:49Z

plugins/faustwp/includes/replacement/functions.php

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


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

Good spot 💯

Updated 👍

whoami-pwd · 2025-01-09T15:31:16Z

plugins/faustwp/includes/replacement/functions.php

+		return apply_filters( 'faustwp_get_wp_site_urls', array( $site_url ) );
+	}
+
+	$is_https = substr( $site_url, 0, 6 ) === 'https:';


strpos can be faster for same purpose.

Thanks good to know 😄

whoami-pwd · 2025-01-09T15:33:03Z

plugins/faustwp/includes/replacement/functions.php

+	$host_url = wp_parse_url( $site_url, PHP_URL_HOST );
+
+	if ( ! is_string( $host_url ) ) {
+		return apply_filters( 'faustwp_get_wp_site_urls', array( $site_url ) );


Maybe we can reduce the function length and get rid off one return?

This probably isn't needed as the site url should be a string anyway.

plugins/faustwp/includes/replacement/callbacks.php

whoami-pwd · 2025-01-09T15:41:34Z

plugins/faustwp/includes/replacement/callbacks.php

-		$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}(\/|$))#";


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.

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 👍

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

plugins/faustwp/includes/replacement/callbacks.php

whoami-pwd · 2025-01-09T15:45:38Z

plugins/faustwp/includes/replacement/callbacks.php

@@ -95,40 +102,43 @@ 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 ) {


I think we need to improve the function a little bit. It is definitely worth to keep initial comments here. And I guess there is a chance for us to make it more readable along with more compact.

Ah that was updated when I ran the phpcbf fix

I will look at seeing what we can improve in terms of readability..

Updated PHPDocs 👍

whoami-pwd · 2025-01-09T15:46:23Z

plugins/faustwp/includes/replacement/functions.php

+	$upload_dir = wp_upload_dir()['baseurl'];
+
+	foreach ( $site_urls as $site_url ) {
+		if ( false !== strpos( $upload_dir, $site_url ) ) {


Are we checking if it starts with or contains?

We cannot use str_starts_with as we need to support PHP 7. I will look at updating this to check if it starts with the site URL

Sorry, I mean that strpos($haystack, $needle) === 0 verifies if it starts with and strpos($haystack, $needle) !== false that it contains.

Co-authored-by: Alex K. <[email protected]>

colinmurphy · 2025-01-09T19:24:35Z

Thanks @whoami-pwd for the great code review 👍

Can you review the changes in the latest commit please.

whoami-pwd

Nice work! I appreciate the updates you've made. There are still a couple of areas where we could improve the code quality/performance. Feel free to reach out if you have any questions.

whoami-pwd · 2025-01-10T11:36:44Z

plugins/faustwp/includes/replacement/functions.php

+ * @return array<string> The array of media Urls
+ */
+function faustwp_get_wp_media_urls( array $wp_site_urls ) {
+	$upload_url = faustwp_get_relative_upload_url( $wp_site_urls );


I think we can just pass the $upload_url here as a parameter, then we won't need to call it once again inside the function.

whoami-pwd · 2025-01-10T11:38:17Z

plugins/faustwp/includes/replacement/functions.php

+ *
+ * @return string The replaced string
+ */
+function faustwp_replace_media_url( string $content, array $wp_media_urls, string $replace_url ) {


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.

whoami-pwd · 2025-01-10T11:40:32Z

plugins/faustwp/includes/replacement/functions.php

+function faustwp_get_wp_media_urls( array $wp_site_urls ) {
+	$upload_url = faustwp_get_relative_upload_url( $wp_site_urls );
+
+	if ( ! is_string( $upload_url ) ) {


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

whoami-pwd · 2025-01-10T11:47:16Z

plugins/faustwp/includes/replacement/callbacks.php

+
+	$wp_media_urls       = faustwp_get_wp_media_urls( $wp_site_urls );
+	$relative_upload_url = faustwp_get_relative_upload_url( $wp_site_urls );
+	$frontend_uri        = faustwp_get_setting( 'frontend_uri' );


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

This is not allowed by our phpcs settings 😅

Sorry I was wrong here.

whoami-pwd · 2025-01-10T11:53:44Z

plugins/faustwp/includes/replacement/callbacks.php

 		'#^/#',
 	);

-	$replacement = $frontend_uri;
+	foreach ( $sources as $width => $source ) {


That would be great to improve this loop. Currently it has some memory/structure issues.
I believe that would be very helpful to have inline comments here to understand what the functionality is doing.

No problem at all. I will add a few comments and review to see if we can remove the loop 👍

Updated. I added a comment about why we need to replace media srcsets when it is disabled.

We need to ensure that the wp site URL is used for both any FE URL or relative URL. The latter is a legacy issue.

plugins/faustwp/includes/replacement/callbacks.php

Fix for #1986. Fixes various issues with content replacement callback…

2be8bfe

… and also updates tests to use WP 6.6 and 6.7 Fixes various issues with using content replacement for the site URL.

colinmurphy changed the title ~~Fix for #1986. Fixes various issues for content replacement for site url (draft)~~ Fixes various issues for content replacement for site url (draft) Dec 24, 2024

Fixed issues with PHPCS

70a67ef

colinmurphy added 2 commits December 30, 2024 14:35

Updated PR to replace srcset URL for different HTTP Protocols

03a33a9

Added changeset

be14c9a

colinmurphy self-assigned this Dec 30, 2024

colinmurphy changed the title ~~Fixes various issues for content replacement for site url (draft)~~ Bug: Fixes issues for content replacement for the site and media URLs Dec 30, 2024

colinmurphy marked this pull request as ready for review December 30, 2024 15:58

colinmurphy requested a review from a team as a code owner December 30, 2024 15:58

colinmurphy mentioned this pull request Dec 30, 2024

Media in ACF WYWIWYG field is serving the front end path instead of the backend path #1956

Open

1 task

josephfusco approved these changes Jan 7, 2025

View reviewed changes

Refactoring based off @whoami-pwd feedback. Reduced some of the nesti…

c6ed6d5

…ng for cotent replacement.

colinmurphy requested a review from whoami-pwd January 8, 2025 18:42

colinmurphy added 2 commits January 8, 2025 19:01

Merge branch 'canary' into bug-fix-url-rewrite-issues

f48551b

Fixes for linting

400e7fc

whoami-pwd requested changes Jan 9, 2025

View reviewed changes

colinmurphy and others added 2 commits January 9, 2025 16:04

Update plugins/faustwp/includes/replacement/callbacks.php

acabb9e

Co-authored-by: Alex K. <[email protected]>

Added fixes as per code review from @whoami-pwd

8e9a232

whoami-pwd requested changes Jan 10, 2025

View reviewed changes

whoami-pwd reviewed Jan 10, 2025

View reviewed changes

plugins/faustwp/includes/replacement/callbacks.php Outdated Show resolved Hide resolved

colinmurphy added 2 commits January 10, 2025 14:05

Refactoring some of the image srcset replacement.

9770673

Small fix for PHPCS for a missing fullstop

71b1d68

colinmurphy added 2 commits January 10, 2025 17:55

Refactoring some of the media urls with @whoami-pwd

58d19df

Fixes

e37f7b0

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

Are you sure you want to change the base?

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

Conversation

colinmurphy commented Dec 24, 2024 • edited Loading

Tasks

Description

Related Issue(s):

Testing

Setup

Tests

Test 1: URL Rewrites Enabled

Test 2: URL Rewrites Enabled and Media Rewrites Disabled

Test 3: URL Rewrites Disabled and Media Rewrites Disabled

Test 4: URL Rewrites Disabled and Media Rewrites Enabled

Screenshots

Documentation Changes

Dependant PRs

changeset-bot bot commented Dec 24, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Dec 24, 2024 • edited Loading

📦 Next.js Bundle Analysis for @faustwp/getting-started-example

colinmurphy commented Dec 24, 2024

colinmurphy commented Dec 24, 2024

theodesp commented Dec 30, 2024

colinmurphy commented Dec 30, 2024

josephfusco left a comment

Choose a reason for hiding this comment

colinmurphy commented Jan 8, 2025

whoami-pwd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinmurphy commented Jan 9, 2025

whoami-pwd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinmurphy commented Dec 24, 2024 •

edited

Loading

changeset-bot bot commented Dec 24, 2024 •

edited

Loading

github-actions bot commented Dec 24, 2024 •

edited

Loading