determineCanonicalRootUrl may cause crash in PHP 8.1

[rgvy]

For some reason determineCanonicalRootUrl(false) returned a null. In the constructor of App, calling urldecode(null) causes a crash in php 8.1. While I'm working to fix my problem with determineCanonicalRootUrl, a crash can be avoided by coalescing urldecode(null ?? '').

[rgvy]

Specifically the problem appears to be with parse_url using the PHP_URL_PATH option. There is no terminal '/' character from the determineCanonicalRootUrl result. It looks like it gets stripped in that method, then parse_url returns null, which crashes urldecode in php 8.1. I'm using https://localhost:8888/ as my APP_PUBLIC_URL.

[ocram]

Thank you!

I don’t think PHP 8.x handles paths and hosts different from PHP 7.x with parse_url. PHP 8.0 only changed the behavior for missing/empty query strings and fragments.

You’re probably talking about App#__construct, right?

Which of the following three lines is failing exactly, and where is the cause of the error in your opinion?

!empty($this->canonicalRootUrl[0]) ? \parse_url($this->canonicalRootUrl[0], \PHP_URL_HOST) : null,

// or

!empty($this->canonicalRootUrl[1]) ? \parse_url($this->canonicalRootUrl[1], \PHP_URL_HOST) : null

// or

$canonicalRootPath = \urldecode(\parse_url($this->canonicalRootUrl[0], \PHP_URL_PATH));

(By the way, any chance you could let this run on port 80 instead of 888?)

[rgvy]

The easiest fix for php 8.1 would be to just coalesce with an empty string like this:

$canonicalRootPath = \urldecode(\parse_url($this->canonicalRootUrl[0], \PHP_URL_PATH) ?? '');

Apologies, but I haven't traced the issue through the determineCanonicalRootUrl method. Apparently it isn't causing me any problems.

[ocram]

Thanks!

I would include the null check, sure, but it would be good to know where exactly it fails and why, and actually understand the behavior and what we want.

As I said, \parse_url(..., \PHP_URL_PATH) should not behave any different on PHP 8.x, compared to 7.x. So any problem that occurs has probably been present on previous PHP versions as well.

[rgvy]

Maybe this has always been a bug that never caused any problems. It looks like determineCanonicalRootUrl always strips the final '/' character, and \parse_url(..., \PHP_URL_PATH) always returns null if there is no final '/' character. In the App constructor, the result is only used to construct a Router (with the local var canonicalRootPath). Because the App's Router is designed to be a root-level router, perhaps it never caused any problems to use a null path in its construction. In the Router, the value is saved to the rootPath which is only used in the createRouteRegex method. Without a root path, the regex would match more (not less) which would hide this bug.