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

weak_vendors mode and phars #24853

Closed
greg0ire opened this issue Nov 7, 2017 · 4 comments
Closed

weak_vendors mode and phars #24853

greg0ire opened this issue Nov 7, 2017 · 4 comments

Comments

@greg0ire
Copy link
Contributor

greg0ire commented Nov 7, 2017

Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version 3.3.*

When introducing this mode, I overlooked the fact that file paths could look like this:

phar:///usr/local/bin/phpunit-5/phpunit/Util/Fileloader.php

In that kind of case, there is not always a way to tell if the deprecation was triggered inside or outside a vendor, so IMO I should add a special case just for phar paths in that method:

$inVendors = function ($path) {
/** @var string[] absolute paths to vendor directories */
static $vendors;
if (null === $vendors) {
foreach (get_declared_classes() as $class) {
if ('C' === $class[0] && 0 === strpos($class, 'ComposerAutoloaderInit')) {
$r = new \ReflectionClass($class);
$v = dirname(dirname($r->getFileName()));
if (file_exists($v.'/composer/installed.json')) {
$vendors[] = $v;
}
}
}
}
$path = realpath($path) ?: $path;
foreach ($vendors as $vendor) {
if (0 === strpos($path, $vendor) && false !== strpbrk(substr($path, strlen($vendor), 1), '/'.DIRECTORY_SEPARATOR)) {
return true;
}
}
return false;
};

Not sure how realpath behave with such paths, BTW

I wonder how / if I'm going to be able to test that though... any ideas?

@stof
Copy link
Member

stof commented Nov 7, 2017

realpath will return false

I think we should consider that any code being inside a phar is vendor code in this mode. When running a testsuite and having code inside a phar, it is much more likely to be the test runner than the code under test (do you actually build a phar before each test execution, taking into account that this cannot be your executable phar if you are doing unit tests ?)

@greg0ire
Copy link
Contributor Author

greg0ire commented Nov 7, 2017

@stof I experienced this bug once with a phar, but non-vendor code: it was eval'd code produced by getMockForAbstractClass, and the class was a non-vendor class.

But I agree with you that in doubt, we should consider it vendor code.

realpath will return false

good to know :/

@greg0ire
Copy link
Contributor Author

greg0ire commented Nov 7, 2017

A simple fix might be to return true if realpath returns false then?

@greg0ire
Copy link
Contributor Author

greg0ire commented Nov 7, 2017

I think I will be able to test this by using eval

nicolas-grekas added a commit that referenced this issue Jan 21, 2018
This PR was merged into the 3.3 branch.

Discussion
----------

Have weak_vendors ignore deprecations from outside

phar:// and eval() can execute code that may or may not come from the vendors.

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | not yet
| Fixed tickets | #24853
| License       | MIT

I haven't managed to get the phar test to pass yet, but before I do, is it ok for me to commit a test phar in Symfony? I'm thinking trust issues? Although the phar is almost plain text.

~~Next, I'm stuck because it looks like symfony does not know how to load classes anymore... should I somehow load `vendor/autoload.php` from the phar or something like that?~~ solved, had nothing to do with that.

Commits
-------

9ce0ae2 Have weak_vendors ignore deprecations from outside
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants