-
Notifications
You must be signed in to change notification settings - Fork 136
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-1228: Fix issue with rewriting links to front end #1624
Conversation
Determines if post and category URLs should link to the WP site.
Determines if WP media URLs should link to the WP site.
…inside `domain_replacement_enabled()`
🦋 Changeset detectedLatest commit: 6b50f68 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
This comment was marked as off-topic.
This comment was marked as off-topic.
@@ -14,7 +14,6 @@ | |||
exit; | |||
} | |||
|
|||
add_filter( 'graphql_request_results', __NAMESPACE__ . '\\url_replacement' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also still testing this removal to ensure nothing broke. The tests pass, but we need to confirm we're not breaking things we're not testing.
@TeresaGobble test for links to files in the media library in WP, as opposed to external URLs. Because of the bug we're fixing in #1625, it's trickier to test this one. I recommend temporarily deactivating the Faust plugin, creating some content that links to posts and media hosted on the WP site, save, and then reactivate Faust, and do your testing. |
Hi @mindctrl! I have some testing results here to share- things are looking good! It seems like all the The items circled in blue are all the items that changed correctly upon switching to this PR's branch. 🎉 This is a hunch, but I think they refer to the aspect ratios in the block: I also saw Theo and Blake examine the database directly in the Adminer on Local, and I'm noting all items are being written to the site domain, armadillo.local: This one's a hard one to parse. I'm wondering if it makes better sense for us to merge the other ticket, and then potentially retest this one with those changes? |
I'm seeing the following: First I deactivated Faust: I can see the following in the GraphQL Query which is correct. All media sources point to the WP URL. Now I enable Faust but I'm not checking any setting: I can see the following in the GraphQL Query: All This the expected behaviour when no setting has been enabled Since we have specifically the Use the WordPress domain for media URLs in post content setting to rewrite those links back: When I enable this setting then I get back the original src: Now we create a new post with Faust Enabled and link in any post content to an item in the media library. We observe the same behaviour when Some observations:
Is this what we want to have? |
@TeresaGobble @theodesp thanks for the reviews. re: |
Here's the setup I've been using to test: Active plugins
Post contentPost ID 45
Post ID 41
Post ID 39
Post ID 34
Post meta for post ID 34
Query used for all tests
Quick link to diff files of these results, for easier comparison locally. #1624 (comment) Results on canary branch1. Post/Cat/Image pointing to WP
Response
2. Post/Cat/Image pointing to Front-end site URL
Response
3. Post/Cat/ pointing to Front-end, Image pointing to WP
Response
4. Post/Cat/ pointing to WP, Image pointing to Front-end URL
Response
Results on this PR branch1. Post/Cat/Image pointing to WP
Response
2. Post/Cat/Image pointing to Front-end site URL
Response
3. Post/Cat/ pointing to Front-end, Image pointing to WP
Response
4. Post/Cat/ pointing to WP, Image pointing to Front-end URL
Response
|
zip of the result diffs posted above. |
@mindctrl that should be OK will check again today and approve. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! It would be nice to get more approvals.
Hi John! You've done an awesome job explaining your test flow and helping me understand the results- thank you so much for your efforts. 💪 I have two questions on the results I'm seeing. I've documented my testing results in a slideshow so I can compare sections a little more readily to test my understanding. Would you please take a look at the two comments I have in the slides here and let me know your thoughts? |
Adjust logic in image_source_srcset_replacement
Tasks
Description
Take two of #1603.
Adjusts the URL replacement logic to avoid rewriting links to files in the uploads dir.
Related Issue(s):
Testing
Bug is described here: #1569
See this comment with info on demo data, queries, and results.
Ensure that links to posts, images, files, etc. are rewritten as expected. Try to break it!