[go: nahoru, domu]

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

Adding pretty permalinks for checkout #2446

Draft
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

dparker1005
Copy link
Member
@dparker1005 dparker1005 commented Apr 21, 2023

All Submissions:

Changes proposed in this Pull Request:

Allows for passing url-encoded level name or level ID. When testing PR, can flush rewrite rules by saving PMPro Page settings.

For example, if your checkout page is at yoursite.com/membership-account/membership-checkout, you should now be able to check out for a specific level by navigating to yoursite.com/membership-account/membership-checkout/[level_id] or yoursite.com/membership-account/membership-checkout/[level_name_encoded]

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you successfully run tests with your changes locally?

Changelog entry

Enter a summary of all changes on this Pull Request. This will appear in the changelog if accepted.

@dparker1005 dparker1005 added this to the 2.11 milestone Apr 21, 2023
@dparker1005 dparker1005 marked this pull request as ready for review April 21, 2023 18:50
Copy link
Contributor
@MaximilianoRicoTabo MaximilianoRicoTabo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works great and looks good to me. Just a couple of minor comments from my side

includes/functions.php Outdated Show resolved Hide resolved
Copy link
Contributor
@JarrydLong JarrydLong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well on my side.

Would it be an option to reference the new URLs from the Membership Levels page as well instead of /?level=X?

@dparker1005
Copy link
Member Author

Works well on my side.

Would it be an option to reference the new URLs from the Membership Levels page as well instead of /?level=X?

Great idea! We would need to decide whether we would want to use /level-name or /level-id, but I think we definitely want to move in that direction

@ideadude
Copy link
Member

I'm thinking of delaying until 3.0 or like a 2.12 release. I think we need the following to merge this:

  1. Add a "slug" setting to levels. It can default to the level name run through sanitize_title(), but should be a separate field that doesn't change if folks change the level name later (unless they opt to also change the slug). A rule of the internet is to keep "permalinks" permanent and we need a slug field (or using level ids) to do that.

  2. We should update all links to levels in the core plugin to link to the pretty version of the URL if pretty permalinks are enabled site wide. Otherwise, this feature isn't really used without some level of setup by admins... and there are existing work arounds to get pretty permalink levels pages you can link to (create a page and put the checkout block in it).

  3. We also should consider limiting the flush_rewrite_rules() calls further. Can we flush the rules only when the settings are updated vs when the settings page is loaded? We've been doing the latter on the payment settings page through the call to the Set Up Apple Pay method in the Stripe class. So maybe it's not a big deal, but ideally we call that function a little as possible.

  4. We should also discuss and figure out how to introduce people to this change... why URLs are changing... have people opt into it somehow. All this makes me think besides just needing more work, we need to push this to 3.0 as a soft breaking change.

@ideadude ideadude modified the milestones: 2.11, 3.0 May 17, 2023
@dparker1005 dparker1005 changed the base branch from dev to v3.0 July 12, 2023 18:53
@dparker1005 dparker1005 marked this pull request as draft August 4, 2023 18:44
@dparker1005
Copy link
Member Author
dparker1005 commented Aug 7, 2023

Add a "slug" setting to levels. It can default to the level name run through sanitize_title(), but should be a separate field that doesn't change if folks change the level name later (unless they opt to also change the slug). A rule of the internet is to keep "permalinks" permanent and we need a slug field (or using level ids) to do that.

Should this be a new column in the pmpro_membership_levels db table or in level meta?

We also should consider limiting the flush_rewrite_rules() calls further. Can we flush the rules only when the settings are updated vs when the settings page is loaded? We've been doing the latter on the payment settings page through the call to the Set Up Apple Pay method in the Stripe class. So maybe it's not a big deal, but ideally we call that function a little as possible.

As written, we only flush rewrite rules when the page settings page is saved or the option to generate pages is selected. Is this ok or should we only flush when the checkout page is specifically changed?

Side note: We need to make sure to also flush rewrite rules when pages are generated by the setup wizard

We should also discuss and figure out how to introduce people to this change... why URLs are changing... have people opt into it somehow. All this makes me think besides just needing more work, we need to push this to 3.0 as a soft breaking change.

If we are releasing in 3.0, do we still need some kind of opt-in system? What are the use cases that would not want pretty permalinks to be an option?

@ideadude
Copy link
Member
ideadude commented Aug 8, 2023

Should this be a new column in the pmpro_membership_levels db table or in level meta?
Yes.

If we are releasing in 3.0, do we still need some kind of opt-in system? What are the use cases that would not want pretty permalinks to be an option?

It seems like we need to have things work without pretty permalinks one. This is still common on new/dev sites and not uncommon on live sites even. (As far as I know.)

Further we want something like ../membership-checkout/?level=1 to redirect to ../membership-checkout/level-slug/ if "pretty permalinks are enabled", aka if the "permalink_structure" option is NOT empty. If it is empty, then we want to just load the page. We can try to figure out how core WP does "canonical redirects". Maybe there are helper functions for this or we can match when they do the redirect. Take care of other parameters like discount_code, we want to append it to the pretty URL in the redirect.

Checkout forms that for some reason posted directly to .../?level=... will break in the redirect I think. OR we could ignore the redirect if the checkout form is being submitted.

Q: What is the SEO impact of 30xing (301? or what does WP do for canonical redirects generally) all the checkout page URLs?

@ideadude
Copy link
Member
ideadude commented Aug 8, 2023

More info.

The current canonical URL of the checkout page is something like this:
<link rel="canonical" href="https://domain.com/membership-checkout/" class="yoast-seo-meta-tag" />

That doesn't include the ?level=x in it. That may be a good or bad thing in the 2.x versions of PMPro.

After this update, the canonical URL will be .../membership-checkout/level-slug/.

When pretty permalinks are off in 2.x there is no canonical link tag set in the HTML.

@ideadude ideadude self-assigned this Aug 8, 2023
@ideadude ideadude modified the milestones: 3.0, 3.1 Nov 4, 2023
@ideadude
Copy link
Member
ideadude commented Nov 4, 2023

We've done some more thinking about this. There is too much risk that whatever we do here gets undone by the 3.1 checkout updates, since we're thinking about a more dynamic checkout page that perhaps won't be tied to a single level.

I think we also want to avoid BuddyPress like "fake" pages, especially as full site editing in WP develops. If people are looking at a page, they will expect it to behave like a page and be editable like a page. Instead here if you end up on the level 1 page and edit it, you would be editing the checkout page for all levels.

Also, the end affect of this is already possible by creating pages as a subpage of checkout and setting the level attribute of the block or custom post value/etc.

@ideadude
Copy link
Member
ideadude commented Nov 4, 2023

I've set the milestone for 3.1. We will reevaluate then.

@dparker1005 dparker1005 changed the base branch from v3.0 to dev April 11, 2024 16:13
@dparker1005 dparker1005 modified the milestones: 3.1, Backlog Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants