E2E theme switch: match incoming theme slug, then optional folder #59851
Conversation
The `optionalFolder` regex part matches paths with a folder, so it will return the first match, which might contain a folder. First try to honor the including theme slug, that is, without a folder, the look for the optional folder path, if any.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
ramonjd
added
[Type] Bug
| // If the theme is not found, try to match the theme slug with a folder. | ||
| if ( ! matchGroup ) { | ||
| matchGroup = html.match( | ||
| `action=activate&stylesheet=${ optionalFolder }${ encodeURIComponent( |
There was a problem hiding this comment.
The second match is to honor the intentions of:
| // so it will return the first match, which might contain a folder. | ||
| // First try to honor the including theme slug, that is, without a folder. | ||
| let matchGroup = html.match( | ||
| `action=activate&stylesheet=${ encodeURIComponent( |
There was a problem hiding this comment.
The first match is to honor any incoming themeSlug when the tester wants to be explicit.
There was a problem hiding this comment.
Just double-checking I'm understanding the logic here. The goal is to:
- First attempt to match without an optional folder (i.e. default to theme included in the WP release)
- Then attempt to match with an optional folder (i.e. default to a theme in a nested location)
- For situations were folks wish to explicitly include a folder, they can include it in the value of
themeSlug?
That sounds pretty reasonable to me, as I imagine the expectation is to go with the direct theme first, unless someone has explicitly declared otherwise 👍
Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com>
Yes, what you wrote is also my understanding (and the intention behind this change). Thanks for the 👀 !! |
There was a problem hiding this comment.
Thanks for confirming, the logic here sounds good to me. Given that it's an optionalFolder, preferencing first checking for a match without a folder I think is a good plan, and if anyone using the function winds up encountering an edge case, it's still possible for them to provide a folder explicitly, so this LGTM! ✨
…rdPress#59851) * Test for incoming theme slug first. The `optionalFolder` regex part matches paths with a folder, so it will return the first match, which might contain a folder. First try to honor the including theme slug, that is, without a folder, the look for the optional folder path, if any. Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org>
What?
I don't know if this PR is good or not, but throwing it out there.
When switching themes in E2E tests,
optionalFolderregex part matches paths with a folder, so it will return the first match, which might contain a folder.The consequence is that the regex will match
somefolder/twentytwentyfourgiven a theme slug oftwentytwentyfour, and therefore tests that wish to test againsttwentytwentyfourwon't be able to becausesomefolder/twentytwentyfourwill be activated instead.Why?
Context: #59792 (comment)
How?
Test for incoming theme slug first.
First try to honor the including theme slug, that is, without a folder, the look for the optional folder path, if any.
Testing Instructions
All E2E tests should pass.