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

Enhancement: pmpro_is_page utility function #761

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

mathieuhays
Copy link
Contributor

I found myself needing this function recently so I assume it could be useful for other people too :-)

Not sure if the naming is ideal. I was tempted to call it is_pmpro_page which would make more sense but it would break the convention followed for function names (starting with pmpro_).

@eighty20results
Copy link
Contributor

I'm not sure how that utility code helps improve code readability/improves functionality?

The is_page() function uses the array values by default (no need for the extra array_values() function call overhead, nor the pmpro_is_page() call itself).

As a result, the following costs fewer CPU cycles, is more performant and doesn't obfuscate the code in any way:
`
global $pmpro_pages;

if ( is_page( $pmpro_pages ) ) {
// Do something PMPro page specific
}
`

@mathieuhays
Copy link
Contributor Author

The is_page() function uses the array values by default (no need for the extra array_values() function call overhead, nor the pmpro_is_page() call itself).

Didn't know that, thanks for mentioning it.

I'm not sure how that utility code helps improve code readability/improves functionality?

I usually avoid using plugin variables directly because their structure may change in future updates. I prefer using documented APIs hence this function for detecting whether a page is handled by Paid Membership Pro or not. For example I prefer using is_page() instead of global $wp_query; $wp_query->is_page(), both are valid but the function call is more practical.

@ideadude
Copy link
Member

Other plugins like bbpress, buddypress, and woocommerce have similar functions. We will take a look at this.

@sc0ttkclark sc0ttkclark added the ✓ Ready for Merge Pull requests that have been reviewed and confirmed by PM as ready for merge label Jan 25, 2022
@sc0ttkclark sc0ttkclark self-assigned this Jan 25, 2022
@ideadude ideadude removed the ✓ Ready for Merge Pull requests that have been reviewed and confirmed by PM as ready for merge label Feb 22, 2022
@ideadude
Copy link
Member

Couple thoughts.

A common issue when checking something like if ( is_page( $pmpro_pages['levels'] ) ) is that if the levels page is undefined it will return true for any page because is_page( 0 ) assumes you're checking for any page.

I am not sure if the is_page() function does something similar for is_page( array( 'levels'=>null, ... ) ). Let's check. If it does, we need to strip out the empty pages from the array before checking.

I also wonder if we should check pmpro_is_checkout or otherwise check for PMPro blocks/shortcodes on the page and consider pmpro_is_page() true e.g. if there is a checkout block on a page that is not the specified checkout page.

It would also be nice to address this issue at the same time. (#1958) Basically allow us to pass in a page key like 'levels' and call pmpro_is_page( 'levels' ) instead of ! empty( $pmpro_pages['levels'] && is_page( $pmpro_pages['levels'] ) )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants