-
Notifications
You must be signed in to change notification settings - Fork 200
Fix: make Give 4.0+ compatible with legacy user roles #8186
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
base: develop
Are you sure you want to change the base?
Conversation
…sulating inside wp hooks
|
@jonwaldstein I started an initial overview, but I will need more time, so I'll keep checking it tomorrow. What I can see now is that in the PR descriptions says that the GiveWP Worker role can't view forms, but in the docs, we have this information for this same role: |
|
@glaubersilva thanks man! take your time. I think that's a typo in the description from my personal assistant 😏 - I'll update it |
glaubersilva
left a comment
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.
@jonwaldstein This is a great and massive work! Thanks for getting some time to tackle this beast. Overall, everything looks right to me. I just left some comments and questions about possible improvements.
|
|
||
| // Check REST API route for post ID (e.g., /wp/v2/pages/123) | ||
| if (defined('REST_REQUEST') && REST_REQUEST) { | ||
| $cachedPostId = $this->getPostIdFromRestRoute(); |
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'm wondering if we can use get_queried_object() or get_queried_object_id(); instead of $this->getPostIdFromRestRoute() since these methods are used inside the rest_get_queried_resource_route() method to compose the route URL.
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 great question! I had to investigate this. Apparently get_queried_object does not work within this request nor will rest_get_queried_resource_route because they operate in a different kind of request geared towards the frontend. I had opus explain this to me in laymans terms because I was confused LOL
Think of WordPress as having two different doors for incoming requests:
Door 1: The Front Door (Normal page visits)
- When someone visits yoursite.com/my-campaign-page/:
- WordPress says "Someone wants to see a page!"
- WordPress looks up which page, loads it, and remembers: "We're showing page
#123" - get_queried_object() asks: "What are we showing?" → WordPress answers: "Page
#123"
Door 2: The Side Door (REST API calls)
- When the block editor calls yoursite.com/wp-json/wp/v2/pages/123:
- WordPress says "This is an API call, not a page visit"
- WordPress hands it off to the REST server, which reads the URL directly
- WordPress never bothers to remember "we're showing page
#123"—it's just an API call - get_queried_object() asks: "What are we showing?" → WordPress says: "¯\(ツ)_/¯ Nothing, this isn't a page visit"
The Bottom Line:
get_queried_object() only works when someone is visiting a page. During an API call, nobody is "visiting" anything—it's just data being requested. WordPress doesn't track it the same way.
So you have to read the URL yourself to find the 123.
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.
Nice analogy! Living and learning. hahaha
|
@glaubersilva thanks man!, ready for re-review 🪇 |
glaubersilva
left a comment
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.
@jonwaldstein Great job man! It's ready to be intensively tested by QA now. 😅 🚀
Resolves: GIVE-3021
Description
This should solve all our Give user role problems 🤞
For context, our custom give user roles have been around since the beginning of time. Over time, we have gotten out of sync with user roles/caps and the features we have. This PR tries to correct that by digging through the depths of GiveWP and updating all the permission checks to utilize a centralized UserPermission facade.
Affects
edit_give_paymentswithview_give_paymentsVisuals
GiveWP Role Permissions Matrix
Donations
Donors
Forms
Reports
Settings & Tools
Testing Instructions
ZIP: https://github.com/impress-org/givewp/actions/runs/20375958542
We need to test every user role scenario outlined here:
https://givewp.com/documentation/core/give-user-roles/
Pre-review Checklist
@unreleasedtags included in DocBlocks