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

Add wp-cron.php to exclusion list for request handler #344

Closed
wants to merge 1 commit into from

Conversation

dsturm
Copy link
Contributor

@dsturm dsturm commented Feb 5, 2024

This PR includes wp-cron.php request in the exclusion list for the request handler introduced in #339.

@iniznet
Copy link
Contributor

iniznet commented Feb 5, 2024

Wouldn't it be better if we also allowed people to customize this through a filter hook, like:

if (Str::contains(
    $request->getRequestUri(),
    apply_filters('acorn/router/ignore_request_paths', [
        '/wp-comments-post.php',
        '/wp-cron.php',
        '/wp-login.php',
        '/wp-signup.php',
        '/wp-admin/',
    ])
)) {
    return; // Let WordPress handle these requests
}

I have a case related to the new router, where I set a cookie through a custom route that has not yet been migrated to the Laravel router and ended up getting cleared by the router.

@retlehs
Copy link
Member

retlehs commented Feb 5, 2024

A filter is a good idea 👍 We also want to move away from the hardcoded paths and use admin_url(), login_url(), etc

@dsturm Were wp-cron.php requests failing for you without this change?

@dsturm
Copy link
Contributor Author

dsturm commented Feb 5, 2024

@retlehs Nope, not yet. But imo the WP Cron shouldn't be handled by anything outside WordPress core - should it?

@retlehs
Copy link
Member

retlehs commented Feb 5, 2024

The paths added in #339 are for files that rely on the use of cookies, so wp-cron.php shouldn't be affected

@broskees mentioned on our Discord server that he might have a different fix to replace the code introduced in #339, since the response classes handle cookies too and the bootloader changes aren't accounting for that

@dsturm dsturm closed this Feb 5, 2024
@dsturm dsturm deleted the fix-router branch February 6, 2024 06:47
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.

3 participants