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
Add generic function to determine if URL is a store page #45299
Conversation
Test Results SummaryCommit SHA: 095c963
To view the full API test report, click here. To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
Hi @rjchow, Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
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.
Code looks good @chihsuan. What about a page with a WooCommerce block, such as the All Product block. Would that count as a store page?
Good question. 👍 I think it would but I'm not very sure. @adrianduffell, what do you think? This makes me think that we may need to add a filter so that 3rd party developers can add their own store pages to the list of store pages. |
It's a good point. I think we could tackle this after the MVP. For now, the "restrict to store pages only" setting will link to external documentation that lists the pages which are included. We could add a note in the documentation about blocks not being supported yet. We would need to come up with a strategy to detect pages with a WooCommerce block without a performance hit on every front-end request. Perhaps we could build an index of block pages each time a page gets saved. The filter also sounds like a good idea 👍 |
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.
I left some comments on possible performance implications, not too sure if they are impactful
* @param string $url URL to normalize. | ||
*/ | ||
private static function get_normalized_url_path( $url ) { | ||
$quey = wp_parse_url( $url, PHP_URL_QUERY ); |
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.
$quey = wp_parse_url( $url, PHP_URL_QUERY ); | |
$query = wp_parse_url( $url, PHP_URL_QUERY ); |
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.
Fixed in d84d9f2
$normalized_path = self::get_normalized_url_path( $url ); | ||
|
||
// WC store pages. | ||
$store_pages = array( |
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.
I think we can add a filter here for extensibility, what do you think?
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.
Sounds good. Added a filter here 095c963 🙂
} | ||
|
||
// Check product, category and tag pages. | ||
$permalink_structure = wc_get_permalink_structure(); |
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.
not sure how big of an impact it is on performance but there's a get_option() call in wc_get_permalink_structure()
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.
That's a good point. I think it's a minor impact. If it becomes a problem, we can optimize it later.
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.
From ChatGPT:
The impact of calling wc_get_permalink_structure()
on performance depends on various factors such as server configuration, database size, and the context in which the function is being called.
-
Caching: If the result of
wc_get_permalink_structure()
is cached, subsequent calls to the function within the same request will have minimal impact on performance. WordPress itself caches the permalink structure, so in most cases, calling this function won't cause a significant performance hit. -
Database: The function involves querying the database to retrieve the permalink structure. The impact may be noticeable on large or busy websites with heavy database loads. However, for most websites, especially those with caching mechanisms in place, the impact should be minimal.
-
Context: If the function is called frequently or in a critical performance path, such as within a loop that executes many times, the cumulative impact may become more significant. In such cases, it's advisable to call the function sparingly or optimize the code structure to minimize redundant calls.
-
Server Resources: The impact also depends on the server resources available. On high-traffic websites or under heavy server load, the impact may be more noticeable compared to low-traffic websites or under light server load.
Overall, calling wc_get_permalink_structure()
typically has a minor impact on performance in most WordPress environments, especially when cached properly. However, it's essential to consider the specific context and optimize code accordingly if performance issues arise.
'shop' => wc_get_page_id( 'shop' ), | ||
'cart' => wc_get_page_id( 'cart' ), | ||
'checkout' => wc_get_page_id( 'checkout' ), | ||
'privacy' => wc_privacy_policy_page_id(), |
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.
not sure how big of an impact it is on performance but there's a get_option() call in wc_privacy_policy_page_id()
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.
eca820e
to
095c963
Compare
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.
Thanks for addressing the feedback!
Submission Review Guidelines:
Changes proposed in this Pull Request:
Closes #45110.
The function
is_store_page
is added toWCAdminHelper
and is used to determine if a URL is a store page.Store pages are defined as:
Additionally, the following autogenerated pages should be included:
The function ignores the domain and protocol of the URL and only checks the path and query string so that users can only pass
$_SERVER['REQUEST_URI']
to the function for checking if the current page is a store page.How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
WooCommerce Smooth Generator
andCode Snippets
plugins/wp-admin/tools.php?page=smoothgenerator
> Generate 10 products OR runwp wc generate products 10
/wp-admin/admin.php?page=snippets
Only run on site front-end
and activateThis is a store page!
red banner is displayed on the store pages (Shop, Cart, Checkout, My Account, Terms and Conditions, Privacy Policy, Product, Product Categories, Product Tags) and not displayed on other pages (Home, Sample Page, etc.)/wp-admin/admin.php?page=wc-settings&tab=advanced
Terms and conditions
page to Sample Page/wp-admin/options-permalink.php
Shop base with category
test-category
Changelog entry
Significance
Type
Message
Comment