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

Fix the condition where it checks if the request is a REST Request #256

Merged
merged 1 commit into from
Sep 3, 2023

Conversation

marioshtika
Copy link
Contributor

@marioshtika marioshtika commented Oct 19, 2022

Related to issue #257

I used the below variables to make it easy to read

$isRestRequestConstantDefined = defined( 'REST_REQUEST' ) && REST_REQUEST;
$isRestRequest = $isRestRequestConstantDefined || strpos( $requested_url, $rest_api_slug );
if ( $isRestRequest && $user ) {
    return $user;
}

If you want it a single line you can use

if ( ( ( defined( 'REST_REQUEST' ) && REST_REQUEST ) || strpos( $requested_url, $rest_api_slug ) ) && $user ) {
    return $user;
}

@marioshtika
Copy link
Contributor Author

Hello @Tmeister,

any update on this pull request?

@marioshtika
Copy link
Contributor Author

@Tmeister any update on this, this is also happening on the default posts API (more and more often), on different websites.

@mam4dali
Copy link

thank you
This fixes the problems of the last three versions
@Tmeister Please merge if possible

@ckubitza
Copy link

ckubitza commented Feb 9, 2023

I also think the current condition is wrong (as of v1.3.2), because we noticed that since v1.3.1 authorized users did not receive non-public data through API anymore. But your fix changes the condition completely, I think a smaller change would be sufficient.
The original condition checks for the constant REST_REQUEST and at least in our tests, this constant is defined after determine_current_user() is called. So the condition is always true.

So I propose to change the condition from

if ( ! defined( 'REST_REQUEST' ) || ! REST_REQUEST || strpos( $requested_url, $rest_api_slug ) === false || $user ) {

to

if ( (defined( 'REST_REQUEST' ) && ! REST_REQUEST) || strpos( $requested_url, $rest_api_slug ) === false || $user ) {

Or maybe the check for this constant can be dropped completely.

@marioshtika
Copy link
Contributor Author

Hello @ckubitza.
Did you test my pull request, is that working for you?
The code you are suggesting, is that in comparison with the core plugin code or my pull request code?
Thank you.

@ckubitza
Copy link

ckubitza commented Feb 9, 2023

Hey @marioshtika,
yes, I tried it, and it is indeed working. But as I don't know all edge cases the original condition is pointing at, I have tried to change it only as little as possible to make it work.
And my suggestion is compared to the original code of the plugin.

@align-systems
Copy link

Thanks for the fix @marioshtika and @ckubitza. I got errors when upgrading to 1.3.2 a little while back. 404 errors updating posts, so I rolled back due to time constraints but upgrading a site today another plugin (Pods) had a clash with the Firebase\JWT namespace so ran the update again which solved that problem but this problem still existed and after searching found this PR so I've patched for now but @Tmeister it would be really good if you could merge a fix for this. Cheers, Vic

@marioshtika
Copy link
Contributor Author

@Tmeister any update on this PR?
Thank you in advance.

@sashagra
Copy link

sashagra commented Apr 4, 2023

@marioshtika thank you. it solve my problem.

@Tmeister can u merge it please

@marioshtika
Copy link
Contributor Author

@Tmeister any update on this pull request.
Is it ok to merge it?
Thank you in advance.

@GForceWeb
Copy link

@Tmeister Would be great if you could merge this. This fix has also worked for me

@sashagra
Copy link

it is not working anymore
what can i do?

@GForceWeb
Copy link

@Tmeister Pinging again on this. Please respond

@Tmeister Tmeister merged commit 2a2386e into Tmeister:develop Sep 3, 2023
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.

7 participants