Don't assume URIs with dots as static (#61286).

[mperrando]

The build-in server, on serving an URI containg dots, behaves exactly as
for an URI without dots. Only if the URI ends with '.php', the server
gives a 404 status if the php file is not present.

Now the behaviour matches the one expected by reading the documentation.

http://php.net/manual/en/features.commandline.webserver.php

[weltling]

Thanks for the report It's documented to accept index.html or index.php by default, or use some script as a router. Where in the doc it is expected to look for an arbitrary script, which is not a router?

Thanks

[weltling]

strlen(dot) could be less than 4.

[mperrando]

In such case the file has not a ".php" extension. What's the problem?

[weltling]

If both are \0 terminated, i meant. Should be, actually, so perhaps no worries.

Thanks.

[weltling]

This part looks not quite correct. Fe how would be /hello.php/world.php handled? Iterating backwards seems to make more sense. Also some backward compatibility might be broken.

[mperrando]

The code before my patch, would raise a 404 unless a file corresponding to the path /hello.php/world.php is not present, because the path contains a dot.

We can choose to make the recursive search for a router script for ANY file that does not exists, and I agree with this idea. I was only trying to be the less disruptive as possible with the previous behaviour.

For what concerns the backward compatibility: the built-in server is only used for development purposes and must not be used in production.

This new implementation serves all the files that the old one did, but it serves also other paths that instead would have got a 404 status.

As an example, Drupal 8 now works, while before it did not.

[mperrando]

To get a better idea of the problem, take a look at the thread of this bug https://bugs.php.net/bug.php?id=61286

In particular to the following statement by laurence.

Built-in server will consider it as a static file request if there is a dot in the SCRIPT_NAME.

so, it's kind of expect behavior. thanks

This assumption is way too loose, because a path like /go.to/here is treated as a static file, giving a 404 if the "here" file is not present. While people wants it to be served by some index.php or index.html file, as any other path that does not correspond to a static file.

This is why, I chose that ONLY in the case when the path ends with ".php" and a corresponding file does not exist, it returns a 404 code, while in all other cases the recursive search for an "index" file must be done.

@weltling I don't understand the "arbitrary script" part. In the former code, if you specify a path that represents an existing PHP script file, this file is run and no recursive search is done. What do you mean? This behaviour should be conserved also in this implementation.

[weltling]

@mperrando ok, thanks for the link. There's a bit of lack on the definition there. Perhaps the documentation doesn't mention it, but the basic dilemma is actually about the non existent path handling. Fe with your current patch - sapi/cli/tests/php_cli_server_016.phpt breaks the currently documented behavior. The router returns false, the file doesn't exist, but the server delivers 200.

Also the other part of the patch /hello.php/world.php - seems it would always prefer hello.php. What if hello.php is a directory? Or, how it would handle the path /hello.php/world.txt where world.txt is a text file. Etc.

The existing mechanism is neither a full featured router framework alike, nor it is a full featured web server. It is also not to expect to be so. IMO it would make sense to first specify the behavior to expect, considering also edge cases. The ideas from the bug #61286 seem to have no big support so far. Implementing a full featured web server just for testing seems too much.

Thanks.

[mperrando]

The implementation was quite strange even before my PR, IMHO.

The problems you are correctly posing ware not considered even before my patch!

This patch is moving things toward a more "correct" behaviour, i.e. the one you obtain on the production site. Anyway, anything has been put under test. In case someone finds a misbehaviour it will ve very simple to add a new test and decide how to modify things.

As I said, as an example, with this patch you can run a Drupal 8 installation, for development purposes, with the built-in server, while you cannot do it now: Drupal 8 uses some path containing "dots", that should be served by the main Drupal index.php script (and are, in production, with a front end web server like Apache), but instead the get a 404 status now.

Furthermore, it does not seem to me that this patch could break things: the paths that were served before are served with this PR too. Actually the number of path served should be only incremented by this PR.

Said this, I don't see any threat in merging: this should help people developing sites with the built-in server in an environment that better resembles the true world of production.

[weltling]

The test api/cli/tests/php_cli_server_016.phpt definitely shows a BC breach. I've read, that it is your first contribution, so please don't let to discourage you but check https://wiki.php.net/rfc/releaseprocess for more info. There are many cases, where people rely on buggy behavior. Saying "app ABC is fixed" means in many cases "beyound app XYZ" is broken.

In some circumstances, patches are under the consideration of the corresponding RMs. IMO, a good way to go would be to create a specification which would consider behaviours when a router is used or not, handling of static paths vs. virtual routing, weird edge cases and beyond. It still wouldn't have to be full featured, but at least the behaviour were discussed and well defined. Otherwise it's to see for more comments on the existing patch.

Thanks.

[krakjoe]

Since there has been no updates for more than 6 months, I'm closing this PR.

If I'm wrong to close it and you would like to continue working on this patch, please reopen the PR or create a fresh one targeting an appropriate branch.

[andypost]

Does it make sense to open new PR? this bug still exists(

[Amunak]

There should definitely be a bug report or something, this makes the internal webserver useless for anything that requires that path contain a dot.