[CMSP-1459] Fixes URL normalization process #138
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
this is a setting that can be changed, so this way we're setting it to what we think it should be without forcing users to use our path
use dynamic site_url and make the setting conditional based on whether there's already a /wp/ i the siteurl
that's a theme thing. the canary sites are running a different theme
so we haven't cd'd into the local copy when we're looking for the commit message
so it's not just truncated if it's long
so the substr check isn't null
use the composer.json instead of composer.lock use the conditional to check for cache
pwtyler
approved these changes
Jul 9, 2024
Comment on lines
+67
to
+73
| if terminus site:info wpcm-playwright-tests; then | ||
| echo "Test site already exists, skipping site creation." | ||
| # If the site exists already, we should switch it to git mode. | ||
| terminus connection:set wpcm-playwright-tests.dev git -y | ||
| else | ||
| terminus site:create wpcm-playwright-tests 'WordPress (Composer Managed) Playwright Test Site' 'WordPress (Composer Managed)' --org=5ae1fa30-8cc4-4894-8ca9-d50628dcba17 | ||
| fi |
There was a problem hiding this comment.
site name and org UUID can/should remain config'd in this file, but can they be abstracted to global variables to ease identification (and replacement)?
There was a problem hiding this comment.
Yes, I was just avoiding it to avoid complexity. We can come back to this to refine it later, I was more concerned that we had tests.
| * @param array $parts URL parts from parse_url. | ||
| * @return string Re-parsed URL. | ||
| */ | ||
| function __rebuild_url_from_parts( array $parts ) : string { |
There was a problem hiding this comment.
feels very dirty that this doesn't exist already 😬
Comment on lines
+206
to
+213
| // Normalize the URL to remove any double slashes. | ||
| if ( isset( $parts['path'] ) ) { | ||
| $parts['path'] = preg_replace( '#/+#', '/', $parts['path'] ); | ||
| } | ||
|
|
||
| // Normalize the remaining URL to remove any double slashes. | ||
| $normalized_url = str_replace( '//', '/', $remaining_url ); | ||
| // Rebuild and return the full normalized URL. | ||
| return __rebuild_url_from_parts( $parts ); | ||
| } |
There was a problem hiding this comment.
seems over-engineered with the same result but maybe I'm missing what's changed and how the bugs manifested.
There was a problem hiding this comment.
I'm not entirely convinced it's necessary but I'm also not convinced that it's not. It was one of the many steps on the path of resolving the bug(s).
There was a problem hiding this comment.
we need tests (I know you know this)
Co-authored-by: Phil Tyler <[email protected]>
…ntheon-systems/wordpress-composer-managed into bugfix-fix-core-resource-url-handling
jazzsequence
added a commit
that referenced
this pull request
Jul 31, 2024
* add new helper function that parses and normalizes a url from parts * Update URL normalization function to refactor using __rebuild_url_from_parts * check if endpoint is defined already * dont' use anonymous function * add host if exists * replace graphql endpoint filter with an action hook to prepopulate this is a setting that can be changed, so this way we're setting it to what we think it should be without forcing users to use our path * move the prepopulate action and make it more robust use dynamic site_url and make the setting conditional based on whether there's already a /wp/ i the siteurl * add exclusions to site_url filter for graphql * add playwright tests * change ci to install * don't create the site if it already exists * add git config for the user * wait for the last action to finish before switching to sftp mode * set pretty permalinks for playwright test site * switch the existing site to git mode * force yes * check the h2 instead of the site title that's a theme thing. the canary sites are running a different theme * test that the resource urls have /wp/ in the path * missing " * expect the resource url to be truthy on a request * append the commit message from the PR to the commit to the test site * use github token so we can use gh * add package.json to export-ignore * move the commit msg var up so we haven't cd'd into the local copy when we're looking for the commit message * we don't need `page` at all * this doesn't change anything other than seeing if we're getting the PR number * fix the commit message * Include the git commit message body so it's not just truncated if it's long * don't fail if there's nothing to commit * add caching for dependencies * fix hello world test * emdash problems * fix the locator on the welcome message test * generate lock files so we can use the cache * pass default values so the substr check isn't null * change test to toContainText * only generate the npm lock use the composer.json instead of composer.lock use the conditional to check for cache * party pooper 💩 Co-authored-by: Phil Tyler <[email protected]> * bail early --------- Co-authored-by: Phil Tyler <[email protected]>
jazzsequence
added a commit
that referenced
this pull request
Aug 1, 2024
* add new helper function that parses and normalizes a url from parts * Update URL normalization function to refactor using __rebuild_url_from_parts * check if endpoint is defined already * dont' use anonymous function * add host if exists * replace graphql endpoint filter with an action hook to prepopulate this is a setting that can be changed, so this way we're setting it to what we think it should be without forcing users to use our path * move the prepopulate action and make it more robust use dynamic site_url and make the setting conditional based on whether there's already a /wp/ i the siteurl * add exclusions to site_url filter for graphql * add playwright tests * change ci to install * don't create the site if it already exists * add git config for the user * wait for the last action to finish before switching to sftp mode * set pretty permalinks for playwright test site * switch the existing site to git mode * force yes * check the h2 instead of the site title that's a theme thing. the canary sites are running a different theme * test that the resource urls have /wp/ in the path * missing " * expect the resource url to be truthy on a request * append the commit message from the PR to the commit to the test site * use github token so we can use gh * add package.json to export-ignore * move the commit msg var up so we haven't cd'd into the local copy when we're looking for the commit message * we don't need `page` at all * this doesn't change anything other than seeing if we're getting the PR number * fix the commit message * Include the git commit message body so it's not just truncated if it's long * don't fail if there's nothing to commit * add caching for dependencies * fix hello world test * emdash problems * fix the locator on the welcome message test * generate lock files so we can use the cache * pass default values so the substr check isn't null * change test to toContainText * only generate the npm lock use the composer.json instead of composer.lock use the conditional to check for cache * party pooper 💩 Co-authored-by: Phil Tyler <[email protected]> * bail early --------- Co-authored-by: Phil Tyler <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request
/wp/graphqlwhile still allowing that option to be set via the plugin options.This resolves an issue where URLs were being interpreted incorrectly. Most notably, WP core library urls (e.g. to files like
/wp-includes/js/dist/interactivity.min.js) were being rendered ashttps://dev-cxr-wpcm-canary-test.pantheonsite.io/wp-includes/js/dist/interactivity.min.js/?ver=6.5.5.Additionally, this should resolve the issues with URL structure causing the canary tests to fail.