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

MERL-1270: Do not rewrite links in the post editor #1625

Merged
merged 8 commits into from
Nov 15, 2023

Conversation

mindctrl
Copy link
Contributor

@mindctrl mindctrl commented Oct 27, 2023

Tasks

  • 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.
  • phpunit tests added

Description

When Faust’s “Enable Post and Category URL rewrites” is enabled, this has been causing URLs to be rewritten in the editor to contain the front-end URL instead of the WP URL, which causes them to be saved to the database this way.

Related: the guid value for the post is also saved with the Faust front-end URL. This should also be saved with the WP domain.

Ideally the WP URLs are saved to the database, so that:

  • The content is consistent
  • Our rewrite logic can properly target all desired URLs

Steps to Reproduce

  • Install and activate the Faust WP plugin
  • Add a front-end URL to the Faust settings. Leave “Enable Post and Category URL rewrites” enabled (which is the default)
  • Create a new post. Add a text link to another post. Note the front-end URL shows in the search results. Save the post.
  • Note the front-end URL was saved in post_content and guid instead of the WP URL.

Expected Behavior

The WP URL is saved to the database, so that the content is consistent and matches the domain/URL where it actually lives.

Testing

  • Check out this branch
  • Create a new post, follow the steps above.
  • Note the links use the WP domain and are saved in post_content and guid.
  • Try to break it! Look for flaws in the logic.

Documentation Changes

Dependant PRs

When Faust’s “Enable Post and Category URL rewrites” is enabled, this has been causing URLs to be rewritten in the editor to contain the front-end URL instead of the WP URL, which causes them to be saved to the database this way. Instead, we want the WP URL saved to the database.
@changeset-bot
Copy link

changeset-bot bot commented Oct 27, 2023

🦋 Changeset detected

Latest commit: 14d8384

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 Patch

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
Copy link
Contributor

github-actions bot commented Oct 27, 2023

📦 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! 🙌

Confirms links are not rewritten on post-new.php pages when $_POST is empty. Also confirms Ajax requests to generate sample permalinks are not rewritten. These checks prevent the incorrect guid from being written to the database.

Confirms `wp-link-ajax` Ajax requests are not rewritten. This is used by the Classic Editor when linking content.

chore: update `enable_rewrites` values in tests to be `1` instead of `true`, for consistency.
@mindctrl mindctrl marked this pull request as ready for review October 31, 2023 16:41
@mindctrl mindctrl requested a review from a team as a code owner October 31, 2023 16:41
@theodesp
Copy link
Member

theodesp commented Nov 2, 2023

I can verify the before and after using a sample post:

Before
I create a post and link to another post. The contents of the database are:

Screenshot 2023-11-02 at 15 08 35

After
I create a post and link to another post. The contents of the database are:

Screenshot 2023-11-02 at 15 10 45

Querying using GraphQLin both cases return the rewrite rule:

Screenshot 2023-11-02 at 15 11 51

@@ -200,13 +200,39 @@ function post_preview_link( $link, $post ) {
* @return string URL used for the post.
*/
Copy link
Member

@theodesp theodesp Nov 2, 2023

Choose a reason for hiding this comment

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

Since we are preventing storing the Headless url in the database I think we need to add the following filter in line 24 for the content_blocks plugin:

add_filter( 'wpgraphql_content_blocks_resolver_content', __NAMESPACE__ . '\\content_replacement');

This is to ensure that we trigger domain replacement when resolving the blocks using parse_blocks.

Without the above filter:
Screenshot 2023-11-02 at 17 08 47

With the above filter:
Screenshot 2023-11-02 at 17 07 49

Unless there is a better way... @josephfusco any hints?

@blakewilson
Copy link
Contributor

Nice work on this @mindctrl! I was also able to confirm what @theodesp is seeing as the correct behavior.

Before:
Screenshot 2023-11-02 at 1 32 09 PM
Screenshot 2023-11-02 at 1 30 21 PM

After:
Screenshot 2023-11-02 at 1 31 50 PM
Screenshot 2023-11-02 at 1 30 54 PM

(Not approving however do the concerns @theodesp raised with the content blocks filter)

@mindctrl
Copy link
Contributor Author

@theodesp @TeresaGobble I just brought this up to date with canary. Can you please take another look?

theodesp

This comment was marked as outdated.

Copy link
Contributor

@blakewilson blakewilson left a comment

Choose a reason for hiding this comment

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

Working good 👍

Screenshot 2023-11-15 at 10 06 19 AM

Screenshot 2023-11-15 at 10 06 01 AM

Tested with links to other posts and images as well.

@mindctrl mindctrl merged commit 75f5c80 into canary Nov 15, 2023
18 checks passed
@mindctrl mindctrl deleted the merl-1270_fix-persisted-links branch November 15, 2023 16:28
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.

4 participants