picocms / Pico

Pico is a stupidly simple, blazing fast, flat file CMS.
http://picocms.org/
MIT License
3.83k stars 614 forks source link

Pico doesn't find vendor/autload.php when installed as composer dependency #285

Closed dmelo closed 8 years ago

dmelo commented 8 years ago

I have Pico as a dependency on my project. It resides on vendor/picocms/pico. When index.php is called, I get an error saying that it cannot find vendor/autoload.php

PhrozenByte commented 8 years ago

First of all: Thank you for your contribution/PR!

Unfortunately I can't see a use case where this is actually a problem. When you use Pico as a dependency, why would you call vendor/picocms/pico/index.php directly? Wouldn't you rather copy it to your projects root directory (i.e. the httpdocs dir of your webserver)? Wouldn't you furthermore change Picos config, plugins and themes dir to bypass the need to change anything inside your vendor directory? I would say (we should discuss this @dmelo, @theshka) that Pico's default index.php is intended to be used for standalone installations and as an example for people using Pico as a dependency, not for calling it through vendor/picocms/pico/index.php. What do you think?

theshka commented 8 years ago

When you put it that way, I would say that Pico's default index.php is intended for standalone installations @PhrozenByte. It's true, I probably don't fully understand what a project that includes Pico the way you described looks like... You say you don't see any use case where someone would call vendor/picocms/pico/index.php directly, I can't say one way or the other :confused:

Why do you call it directly @dmelo?

dmelo commented 8 years ago

I've installed pico and wanted to test if everything was working:

composer require picocms/pico
cd vendor/picocms/pico
php -S 0.0.0.0:8899

When I accessed http://localhost:8899 however, it was giving an error message. Saying it couldn't find vendor/autoload.php. Just thought it would be nice if Pico worked right away even installed that way.

For debugging, while I'm implementing my own public/index.php (which creates a instance of Pico), it is good to be able to compare it with a working instance (php -S on the vendor/picocms/pico).

PhrozenByte commented 8 years ago

Hmm... Okay, basically this seems to be a valid use case.

Unfortunately I'm not very pleased with the idea of haphazardly looking for a autoload.php in ../../../vendor/ - a application should never assume anything about the directory structure below its own root directory, this can drastically go wrong and even cause security problems. Hence we need a way to safely distinct between a regular standalone installation and the use as library - but without heavily increasing complexity.

Just to be clear: We absolutely want to support the use of Pico as library, that's the reason why we specify "type": "library" instead of "type": "project" in our composer.json. Any ideas?

@dav-m85, as our packagist/composer/travis specialist, do you have an idea?

theshka commented 8 years ago

While I agree it feels haphazard to assume anything about the directory structure above Pico, it appears that others deal with this issue similarly to my first thoughts in https://github.com/picocms/Pico/pull/286#issuecomment-159107251 @PhrozenByte

See: https://github.com/zendframework/ZFTool/pull/31/files https://github.com/BaunCMS/Baun/blob/master/baun https://github.com/Behat/Behat/blob/master/bin/behat#L21 https://github.com/lassodatasystems/LassoMultilogBundle/commit/56203d5250bfcd5f9e9d6a0f954136709df5a88d

I'd also be interested to know if anyone else has another method for determining if this is indeed a standalone installation or being used as a library.

PhrozenByte commented 8 years ago

The examples are a rather unfortunate choice (ZFTool and Behat are "binaries", thus they are usually used through vendor/bin anyway; LassoMultilogBundle shows the bootstrap script for unit tests; Baun also expects just vendor/autoload.php), but howsoever, you're completely right when saying that others do it that way. Nevertheless I think it's a very very bad idea to do it that way... :wink:

It's furthermore a little bit self-contradictory: we try to work around a non-default use case by assuming defaults - just consider a "config": { "vendor-dir": "a-custom-vendor/path/" } in the composer.json of the root project.

theshka commented 8 years ago

Not the most shining examples, no. :laughing:

Albeit I'm out of my depth here, but if you add "config": { "vendor-dir":... to the root project composer.json how does Pico take that into account in index.php? Can something like $composer->getConfig()->get('vendor-dir');be called?

PhrozenByte commented 8 years ago

Pico doesn't take this into account, it simply doesn't work :smile: As far as I know it's not possible to get any information about composer on runtime, especially not without knowing where the autoloader.php is. But maybe I'm just missing something... Maybe someone else knows more.

theshka commented 8 years ago

It seems nobody from the Composer project can provide a more detailed or better way of doing it.... :astonished: Here's some discussions I found:

https://github.com/composer/composer/pull/234

"I think the correct way is to include DIR.'/../vendor/.composer/... or DIR.'/../../../.composer/..., much like the solution ... Because that way you account for "installs as a top level project" and "installs as a dependency within $vendor/composer/composer/". It's not super nice but I honestly don't know what else to do." - Repo Owner

https://github.com/composer/composer/issues/2904

it should actually handle COMPOSER_VENDOR_DIR more gracefully, or at all even. Something like this should do the trick:

if (getenv('COMPOSER_VENDOR_DIR') && is_file(__DIR__ . '/../' . getenv('COMPOSER_VENDOR_DIR') . '/autoload.php')) {
    require(__DIR__ . '/../' . getenv('COMPOSER_VENDOR_DIR') . '/autoload.php');
} elseif (is_file(__DIR__ . '/../vendor/autoload.php')) {
    require(__DIR__ . '/../vendor/autoload.php');
} elseif (is_file(__DIR__ . '/../../../autoload.php')) {
    require(__DIR__ . '/../../../autoload.php');
} else {
    // ...

...they mostly are talking about "binaries" too, I guess.

https://github.com/composer/composer/issues/3645

...

Well how do you explain this then :

// installed via composer?
if (file_exists($a = __DIR__.'/../../../../../autoload.php')) {
    require_once $a;
} else {
    require_once __DIR__.'/../vendor/autoload.php';
}

... first case is about being installed as a dependency (locally or globally does not matter) while the second case is about being the root package ... So basically, having both always make sense right ? ... yes.

... every answer has a different amount of level changes :joy:

dmelo commented 8 years ago

I have changed the way to look for autoload.php, on the PR.

PhrozenByte commented 8 years ago

I'll merge your PR, albeit grudgingly. It seems to be the only solution, even this still remains a very bad practice and possible security flaw (we, at least, can pin to composer...). Anyway, that's not your fault, thanks for your contribution @dmelo! :+1: :smiley:

@theshka, I'm thinking about adding a different index.php without path searching for our pre-built packages (i.e. a index.php.dist that is added as index.php to our release packages on Travis). What do you think?

theshka commented 8 years ago

@PhrozenByte, if it could be baked in to our deploy script, that would actually be a very smart solution to the problem and potential security issues while not affecting composer users... It would just sit in the build directory?

:+1: for that idea!!!

dav-m85 commented 8 years ago

Sorry for the lag, busy BF week :)

I join @PhrozenByte on the subject, this is terribly bad practice to allow loading composer from another path from within a library, with the clear exception of phar archives.

Reasons are numerous, but the three mains are:


{
    "config": {
        "vendor-dir": "composer_plugin"
    }
}

Will you then add another exception to the if else if else if else if loading ? I even not mention global install where pico would be in a totally different directory.

tl;dr; @dmelo As mentioned earlier, copy the index.php in your root, rename it test.php and use it as is to check your design. NEVER execute code directly from within vendor. Many things could happen, including loss of hair and appetite for destruction.

Did i quote GnR ?