8000 [FrameworkBundle] Fix built-in server command for PHP > 5.4.1 by ajessu · Pull Request #4484 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[FrameworkBundle] Fix built-in server command for PHP > 5.4.1 #4484

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 19, 2012
Merged

[FrameworkBundle] Fix built-in server command for PHP > 5.4.1 #4484

merged 1 commit into from
Jun 19, 2012

Conversation

ajessu
Copy link
Contributor
@ajessu ajessu commented Jun 2, 2012

Bug fix: yes
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes

License of the code: MIT

The router isn't routing with PHP > 5.4.1, unless you explicitly include the name of the controller.

For the default command: app/console server:run

localhost:8000/app_dev.php Works
localhost:8000/ Doesn't work (it used to work on PHP 5.4.0)

There was a change after PHP 5.4.1 which makes the router from the built-in server command not work, when no resource is specified, as the variable $_SERVER['SCRIPT_FILENAME'] passes the isset check.

Changelog: http://php.net/ChangeLog-5.php#5.4.1

  • Implemented #60850 (Built in web server does not set $_SERVER['SCRIPT_FILENAME'] when using router)

The router used to rely on the $_SERVER['SCRIPT_FILENAME'] being set, to return any asset/file if it existed.

This behavior was changed, so that when using PHP's built-in server, the $_SERVER['SCRIPT_FILENAME'] is now populated with a combination of the document root and the router filename
Patch: https://bugs.php.net/patch-display.php?bug_id=60850&patch=add_router_script_file_name_svr_var&revision=latest)

@travisbot
Copy link

This pull request passes (merged b85ff7dd into 1541fe2).

@ajessu
Copy link
Contributor Author
ajessu commented Jun 3, 2012

Thinking a bit more about this, as I find my solution a bit weird.

I'll do a bit more testing and report back.

@fabpot
Copy link
Member
fabpot commented Jun 13, 2012

Any news on this PR?

@travisbot
Copy link

This pull request fails (merged d982bac into 086ff48).

@ajessu
Copy link
Contributor Author
ajessu commented Jun 18, 2012

Updated the solution, and for me, it's ready to be merged now.

I talked about this on Symfony Live with @michal-pipa (the original contributor to this command) and we were trying to avoid touching the filesystem for the check of file existance to skip the server, but I don't think there is any other way (and it's dev mode, it really shouldn't matter much either).

PHP 5.4.1 and up has changed the behavior of $_SERVER['SCRIPT_FILENAME'] for the built-in server and it's a bit unreliable/ugly to rely on it now.

This fixes the command, it works again for all versions of PHP 5.4.x
A very similar solution was also suggested on internals:
http://news.php.net/php.internals/53870

@michal-pipa any other ideas?

@michalpipa
Copy link
Contributor

I'll take a closer look at this tomorrow.

But I think that you should revert to original behavior and call production front controller by default to be consistent with other servers.

@ajessu
Copy link
Contributor Author
ajessu commented Jun 19, 2012

But I think that you should revert to original behavior and call production front controller by default to be consistent with other servers.

I disagree. This is a development-only server, and thus, the development controller should be called since IMHO it will be the most common use case for the command, development.
If for some reason, someone wants to use their production controller, it's as easy as providing a new router and passing it to the command explicitly.

Let me know if you come up with something else.

fabpot added a commit that referenced this pull request Jun 19, 2012
@fabpot
merged branch ajessu/php54-server-fix (PR #4484)
3f44bba 
Commits
-------

d982bac Fix built-in server for PHP > 5.4.1

Discussion
----------

[FrameworkBundle] Fix built-in server command for PHP > 5.4.1

Bug fix: yes
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes

License of the code: MIT

The router isn't routing with PHP > 5.4.1, unless you explicitly include the name of the controller.

For the default command: `app/console server:run`

localhost:8000/app_dev.php `Works`
localhost:8000/ `Doesn't work (it used to work on PHP 5.4.0`)

There was a change after PHP 5.4.1 which makes the router from the built-in server command not work, when no resource is specified, as the variable `$_SERVER['SCRIPT_FILENAME']` passes the `isset` check.

Changelog: http://php.net/ChangeLog-5.php#5.4.1
- Implemented #60850 (Built in web server does not set $_SERVER['SCRIPT_FILENAME'] when using router)

The `router` used to rely on the `$_SERVER['SCRIPT_FILENAME']` being set, to return any asset/file if it existed.

This behavior was changed, so that when using PHP's built-in server, the `$_SERVER['SCRIPT_FILENAME']` is now populated with a combination of the document root and the router filename
Patch: https://bugs.php.net/patch-display.php?bug_id=60850&patch=add_router_script_file_name_svr_var&revision=latest)

---------------------------------------------------------------------------

by travisbot at 2012-06-02T09:06:05Z

This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1506479) (merged b85ff7dd into 1541fe2).

---------------------------------------------------------------------------

by ajessu at 2012-06-03T07:16:33Z

Thinking a bit more about this, as I find my solution a bit weird.

I'll do a bit more testing and report back.

---------------------------------------------------------------------------

by fabpot at 2012-06-13T14:30:28Z

Any news on this PR?

---------------------------------------------------------------------------

by travisbot at 2012-06-18T21:20:17Z

This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1650548) (merged d982bac into 086ff48).

---------------------------------------------------------------------------

by ajessu at 2012-06-18T21:35:44Z

Updated the solution, and for me, it's ready to be merged now.

I talked about this on Symfony Live with @michal-pipa (the original contributor to this command) and we were trying to avoid touching the filesystem for the check of file existance to skip the server, but I don't think there is any other way (and it's dev mode, it really shouldn't matter much either).

PHP 5.4.1 and up has changed the behavior of `$_SERVER['SCRIPT_FILENAME']` for the built-in server and it's a bit unreliable/ugly to rely on it now.

This fixes the command, it works again for all versions of PHP 5.4.x
A very similar solution was also suggested on internals:
http://news.php.net/php.internals/53870

@michal-pipa any other ideas?

---------------------------------------------------------------------------

by michal-pipa at 2012-06-18T23:14:36Z

I'll take a closer look at this tomorrow.

But I think that you should revert to original behavior and call production front controller by default to be consistent with other servers.

---------------------------------------------------------------------------

by ajessu at 2012-06-19T08:48:17Z

> But I think that you should revert to original behavior and call production front controller by default to be consistent with other servers.

I disagree. This is a development-only server, and thus, the development controller should be called since IMHO it will be the most common use case for the command, development.
If for some reason, someone wants to use their production controller, it's as easy as providing a new router and passing it to the command explicitly.

Let me know if you come up with something else.
@fabpot fabpot merged commit d982bac into symfony:master Jun 19, 2012
@michalpipa
Copy link
Contributor

I disagree. This is a development-only server, and thus, the development controller should be called since IMHO it will be the most common use case for the command, development.

Well, if you want dev environment, you should use app_dev.php front controller explicitly, like you do in every other server. I see no reason to call dev controller by default in PHP built-in server and prod controller in any other server. This is inconsistent and for sure it will cause confusion to users: "Why dev environment is used in this case? Have I done something wrong?"

If you have multiple environments like: dev, staging and prod, and use built-in server in one of them and Apache in the others, then the same URL can use different controllers and you have to be aware of which environment use which server. I don't like this.

Last thing: using built-in server in production is not the same thing as using prod front controller in development environment. I don't think that there's something wrong with this.

@ajessu
Copy link
Contributor Author
ajessu commented Jun 19, 2012

Well, if you want dev environment, you should use app_dev.php front controller explicitly, like you do in every other server. I see no reason to call dev controller by default in PHP built-in server and prod controller in any other server. This is inconsistent and for sure it will cause confusion to users: "Why dev environment is used in this case? Have I done something wrong?"

Explained like that, I agree completely with you that the default controller should be the prod one.
However, I went back to the pull request I made to change it from the prod environment to the dev environment, and realized something else, which was why I submitted the pull request in the first place.

By default, the symfony-standard edition, doesn't have a / route (only dev does). So by default, the user running a the server:run command would get a 404, which might be a bigger wtf.
With the default router pointing to the dev controller (and thus having a / route) you can also do this, which IMHO is awesome:

git clone https://github.com/symfony/symfony-standard . && composer.phar install && php app/console server:run

And you have symfony running on localhost:8000 without doing anything and only running one command.

@michalpipa
Copy link
Contributor

By default, the symfony-standard edition, doesn't have a / route (only dev does). So by default, the user running a the server:run command would get a 404, which might be a bigger wtf.

I have to check it one more time with your fix, because before the change in PHP code it was working for me.

The other problem is that it is not easy to write tests for this command.

@flojon
Copy link
Contributor
flojon commented Jun 22, 2012

This doesn't work for urls containing query params since REQUEST_URI will not be an existing file anymore.
So you get 404's if you include javascripts/stylesheets with query params.

@flojon
Copy link
Contributor
flojon commented Jun 22, 2012

I just changed REQUEST_URI to SCRIPT_NAME and it seems to work:

if (is_file($_SERVER['DOCUMENT_ROOT'] . DIRECTORY_SEPARATOR . $_SERVER['SCRIPT_NAME'])) {
    return false;
}

@flojon
Copy link
Contributor
flojon commented Jun 23, 2012

I sent a new PR for this: #4638

@ajessu ajessu mentioned this pull request Jun 23, 2012
Merged
fabpot added a commit that referenced this pull request Jul 1, 2012
@fabpot
merged branch flojon/patch-1 (PR #4638)
85d77c2 
Commits
-------

eb26e89 [FrameworkBundle] Fix built-in server when using query params in paths

Discussion
----------

[FrameworkBundle] Fix built-in server when using query params in paths

$_SERVER['REQUEST_URI'] will contain the query params so is_file will fail.
I propose to use $_SERVER['SCRIPT_FILENAME'] instead which contains the full path and no query params.

---------------------------------------------------------------------------

by ajessu at 2012-06-23T10:17:34Z

I was going to make this comment on your approach in #4484, but I'll make it here, since that issue is already closed.

Your solution won't work on PHP 5.4.0, as `$_SERVER['SCRIPT_FILENAME']` will not be set [see PHP bug #60850](https://bugs.php.net/bug.php?id=60850).

Also PHP 5.4.1 and up, if you don't request a file explicitely, Ex:

     http://localhost:8000/app_dev.php

but a location, Ex:

    http://localhost:8000/

The value of the `$_SERVER['SCRIPT_FILENAME']` will be the router file, not the script name, which makes relying on `$_SERVER['SCRIPT_FILENAME']` inconsistent. [See this comment on the php bug](https://bugs.php.net/bug.php?id=60850#1331261652)

I'm not sure if (nor how?) the issue of the params should be addressed on this "default" router, to not make it overly complex.

For your use case, and this is just my own early opinion without much thought, in case we can't come up with a general solution, there is always the option of defining your own router and passing it to the `server:run` command with `--router` like so:

    php app/console server:run --router=app/config/my_own_router.php

---------------------------------------------------------------------------

by flojon at 2012-06-23T10:31:47Z

So would `$_SERVER['SCRIPT_NAME']` be more reliable? Like this:

    if (is_file($_SERVER['DOCUMENT_ROOT'] . DIRECTORY_SEPARATOR . $_SERVER['SCRIPT_NAME'])) {
        return false;
    }

I did a simple test and `$_SERVER['SCRIPT_NAME']` is set to `/` when accessing the root (using PHP 5.4.3).

---------------------------------------------------------------------------

by flojon at 2012-06-23T10:51:22Z

Browse around the code a bit and `$_SERVER['SCRIPT_NAME']` has been there since PHP 5.4.0:
https://github.com/php/php-src/blob/php-5.4.0/sapi/cli/php_cli_server.c#L598

---------------------------------------------------------------------------

by travisbot at 2012-06-23T11:16:59Z

This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1688361) (merged eb26e89 into 0d4b02e).

---------------------------------------------------------------------------

by travisbot at 2012-06-24T10:23:52Z

This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1688043) (merged 71855665 into 0d4b02e).

---------------------------------------------------------------------------

by CHH at 2012-06-29T07:17:32Z

This works fine for me!

:+1:

Could someone please merge this? This issue makes the `server:run` command currently quite unusable, because it can't load CSS for example which has a `?v=` parameter.

---------------------------------------------------------------------------

by ajessu at 2012-06-29T08:25:14Z

:+1: from me also. Works just like `$_SERVER['REQUEST_URI']`, but doesn't include the params.

Tested working on PHP 5.4.0 and 5.4.3.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ajessu @travisbot @fabpot @michalpipa @flojon
0