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

Broken subroute pattern #172

Open
latdev opened this issue Dec 18, 2021 · 5 comments
Open

Broken subroute pattern #172

latdev opened this issue Dec 18, 2021 · 5 comments

Comments

@latdev
Copy link

latdev commented Dec 18, 2021

Looks like getBasePath bug.

With router definition.
$router->get("/backend/io\.js", 'IoController@actionIndex');

Inside Router.class
private function patternMatches($pattern, $uri, &$matches, $flags)

After executing with

      var_dump($uri);
      // we may have a match!
      return boolval(preg_match_all('#^' . $pattern . '$#', $uri, $matches, PREG_OFFSET_CAPTURE));

Pattern fails with $uri.

string(15) "io.js"
@uvulpos
Copy link
Collaborator

uvulpos commented Dec 18, 2021

Please do not send JavaScript files via the router! Configure a public folder on your server where you can store static files. If the request fails or the index file gets executed specifically, execute the router 🙂

/js/file.js -> no router
/user/bla -> router
/index.php/user/bla -> router

@kktsvetkov
Copy link

@uvulpos the backend/io.js might be a server-side generated JS content from PHP, and not a static file.

@uvulpos
Copy link
Collaborator

uvulpos commented Dec 29, 2021

@kktsvetkov

Normally you do not generate Javascript via PHP and even if you would because you create a Javascript generator website, than it would be possible to name it just something e.x. /download and change the output name by PHP header to e.x. something.js. PHPSpreadsheet does this for example if you want to stream your output.

Example:
https://stackoverflow.com/a/2016016
https://stackoverflow.com/q/58971049

Anyway, if this is such a big deal for you, I can try to fix that by pull request but feel free to do it youself and reference this issue by commit ;)

@kktsvetkov
Copy link

@latdev you've probably thought of this already but anyway: as a workaround you can use setBasePath to explicitly set the base path and not rely on getBasePath.

@kktsvetkov
Copy link

@uvulpos The issue is not related directly to JS though, is it? It's about that getBasePath strips the filename from $_SERVER['SCRIPT_NAME']

// Check if server base path is defined, if not define it.
if ($this->serverBasePath === null) {
    $this->serverBasePath = implode('/', array_slice(explode('/', $_SERVER['SCRIPT_NAME']), 0, -1)) . '/';
}

BTW Have you considered using dirname instead of this combination of implode, explode and array_slice here? One function call instead of 3, it is surely going to be faster. I did a quick phpbench test like this:

/**
* @Revs(8000)
* @Iterations(5)
*/
class benchmark_dirname
{
	public $uri = '/bramus/router/issues/172';

	function benchDirname()
	{
		$basePath = dirname($this->uri) . '/';
	}

	function benchImplodeSlideExplode()
	{
		$basePath = implode('/', array_slice(explode('/', $this->uri), 0, -1)) . '/';
	}
}

The dirname approach is 2.3x faster

$ php vendor/bin/phpbench run lab/dirname.php --report=short
PHPBench (1.2.3) running benchmarks...
with configuration file: phpbench.json
with PHP version 7.4.27, xdebug ✔, opcache ❌

\Benchmark_dirname

    benchDirname............................I4 - Mo0.580μs (±4.34%)
    benchImplodeSlideExplode................I4 - Mo1.296μs (±3.58%)

Subjects: 2, Assertions: 0, Failures: 0, Errors: 0
Benchmark_dirname
+--------------------------+---------+---------+---------+---------+---------+--------+
| subject                  | best    | mean    | mode    | worst   | stdev   | rstdev |
+--------------------------+---------+---------+---------+---------+---------+--------+
| benchDirname             | 0.535μs | 0.569μs | 0.580μs | 0.597μs | 0.025μs | ±4.34% |
| benchImplodeSlideExplode | 1.274μs | 1.318μs | 1.296μs | 1.404μs | 0.047μs | ±3.58% |
+--------------------------+---------+---------+---------+---------+---------+--------+

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

No branches or pull requests

3 participants