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 exception thrown in subfolders and malformed paths (Fixes #355) #356

Merged
merged 5 commits into from
Mar 15, 2024

Conversation

Log1x
Copy link
Member

@Log1x Log1x commented Mar 14, 2024

This hopefully fixes all the issues related to routing from the v4 release.

Change log

  • 🐛 Fix exception thrown in subfolders and malformed paths (Fixes Admin pages are broken in multisite with subfolder structure #355)
  • 🩹 Fix canonical redirects on the WordPress REST route
  • 🎨 Add missing types to Bootloader methods
  • 🎨 Utilize the app property in Bootloader methods/properties
  • 🎨 Clean up storage directory separators
  • 🧑‍💻 Properly exclude filtered admin, login, and registration URLs from routing
  • 🧑‍💻 Improve checking against the WordPress REST route
  • 🧑‍💻 Bail on routing when route is a direct .php file
  • 🚩 Remove the experimental request handler flag
    • This needs more testing[1] but I do not want it holding up this PR.

🩹 Fix canonical redirects on the WordPress REST route
🎨 Add missing types to Bootloader methods
🎨 Utilize the `app` property in Bootloader methods
🎨 Clean up storage directory separators
🧑‍💻 Properly exclude filtered admin, login, and registration URLs from routing
🧑‍💻 Improve checking against the WordPress REST route
🧑‍💻 Bail on routing when route is a direct `.php` file
🚩 Remove the experimental request handler flag
Copy link
Member

@QWp6t QWp6t left a comment

Choose a reason for hiding this comment

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

LGTM! :shipit:

@oxyc
Copy link
Contributor

oxyc commented Mar 14, 2024

I can confirm this fixes the exception thrown on URLs like /tuote/leitenberger-rfm-03_%AB-refraktometri-adblue-urealiuokselle/

@oxyc
Copy link
Contributor

oxyc commented Mar 14, 2024

Actually this introduces another bug where a cookie I used to set on a POST request during wp_loaded doesnt get set if WC sets wp_woocommerce_session_X later. If wp_woocommerce_session_X already exists (so on triggering twice) it sets it.

@Log1x
Copy link
Member Author

Log1x commented Mar 14, 2024

Actually this introduces another bug where a cookie I used to set on a POST request during wp_loaded doesnt get set if WC sets wp_woocommerce_session_X later

Do you have an easy way I could test/reproduce this?

@Log1x
Copy link
Member Author

Log1x commented Mar 14, 2024

I added back the ACORN_ENABLE_EXPERIMENTAL_WORDPRESS_REQUEST_HANDLER flag for enabling WordPress request handling until I can reproduce/fix this. I do not want it holding up this PR as it has other important fixes (#355) and the request implementation as-is (with flag) is broken entirely.

@oxyc
Copy link
Contributor

oxyc commented Mar 14, 2024

It's reproducible with:

add_action('wp_loaded', function () {
    setcookie('test1', 1, [
        'expires' => 0,
        'secure' => true,
        'path' => COOKIEPATH ? COOKIEPATH : '/',
        'domain' => COOKIE_DOMAIN,
    ]);
    setcookie('test2', 1, [
        'expires' => 0,
        'secure' => true,
        'path' => COOKIEPATH ? COOKIEPATH : '/',
        'domain' => COOKIE_DOMAIN,
    ]);
});

Only second cookie gets added. Guess the Set-Cookie header just gets overridden

@QWp6t
Copy link
Member

QWp6t commented Mar 14, 2024

I got a fix implemented for the cookie issue.

Response::header() takes a third parameter to indicate whether it should replace any existing headers with the same key.

class Response {
    public function header($key, $values, $replace = true)
}

I still replace by default, unless the header is Set-Cookie.

                    $response->header($header, $value, $header !== 'Set-Cookie');

So we effectively allow multiple Set-Cookie headers to be sent.

image

@oxyc
Copy link
Contributor

oxyc commented Mar 15, 2024

Fix works for me, E2E tests passing again.

@QWp6t QWp6t merged commit dc3cff1 into main Mar 15, 2024
2 checks passed
@QWp6t QWp6t deleted the fix/http-integration-exceptions branch March 15, 2024 13:40
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.

Admin pages are broken in multisite with subfolder structure
3 participants