Read vendor directory from project's composer.json, if set. #281

wulftone · 2017-02-06T03:20:19Z

If not set, default to "vendor".

This PR also includes a bugfix for finding the appropriate composer.json and composer.lock files.

This PR does not address an existing issue with files that are included in composer.json via the "repositories" mechanism with a type of "package" mechanism, e.g.:

        {
            "type": "package",
            "package": {
                "name": "ezyang/htmlpurifier",
                "version": "4.6.0",
                "dist": {
                    "url": "https://github.com/ezyang/HTMLPurifier/archive/v4.6.0.zip",
                    "type": "zip"
                }
            }
        }

These are not currently cached. Those files are stored in the [vendor]/composer/[some hash]/[packageName]-[packageVersion] directory. A future enhancement could cache these files as well.

If not set, default to "vendor". Also sort found vendor directories so we obtain the correct composer.json and composer.lock files

codecov · 2017-02-06T04:31:02Z

Codecov Report

Merging #281 into master will not impact coverage by -0.03%.

@@             Coverage Diff              @@
##             master     #281      +/-   ##
============================================
- Coverage      86.1%   86.07%   -0.03%     
+ Complexity      723      718       -5     
============================================
  Files            54       53       -1     
  Lines          1461     1458       -3     
============================================
- Hits           1258     1255       -3     
  Misses          203      203

Impacted Files	Coverage Δ	Complexity Δ
src/PhpDocument.php	`84.33% <ø> (-0.55%)`	`28 <ø> (-1)`
src/Server/TextDocument.php	`98.01% <100%> (ø)`	`36 <ø> (ø)`	❌
src/Server/Workspace.php	`88.23% <100%> (+0.73%)`	`24 <1> (ø)`	❌
src/utils.php	`100% <100%> (ø)`	`0 <ø> (ø)`	❌

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 571b26a...0ad8f34. Read the comment docs.

felixfbecker

There are other places in the language server where a vendor directory is assumed, those would have to be changed too.

felixfbecker · 2017-02-06T08:50:47Z

src/Indexer.php

            /** @var string[][] */
            $deps = [];
+
+            $vendorDir = str_replace('/', '\/', @$this->composerJson->config->{'vendor-dir'} ?: 'vendor');


don't use the @ operator, check this with if/isset

felixfbecker · 2017-02-06T08:53:37Z

src/Indexer.php

                        $this->client->window->logMessage(MessageType::INFO, "Storing $packageKey in cache");
                        $this->cache->set($cacheKey, $index);
+                    } else {
+                        $this->client->window->logMessage(MessageType::INFO, "Cannot cache $packageName, cache key was null. Either you are using a 'dev-' version or your composer.lock is missing references.");


Use a warning and shorten text to Could not compute cache key for $packageName

I thought including a potential reason would help some people. I can shorten it though.

Logs are meant for developers and admins

felixfbecker · 2017-02-06T08:56:25Z

src/LanguageServer.php

                    $composerJsonFiles = yield $this->filesFinder->find(Path::makeAbsolute('**/composer.json', $rootPath));
+
+                    // If we sort our findings by string length (shortest to longest),
+                    // the first entry will be the project's root composer.json.


This is not true

/myproject/http_rest_api_backend/composer.json /myproject/vendor/psr/log/composer.json

Good call, should have thought about it for more than two seconds. I think substr_count($a, '/') will do it correctly?

Yes, that would work! 👍
But you need to apply it to the path component of the URI, otherwise a URL with host will have 2 more slashes than a URN. Use Uri\parse()

I don't see a Uri class included. Do you have a parsing library to recommend adding to the project?

You will need to add use Sabre\Uri

Change log message

felixfbecker · 2017-02-06T21:21:11Z

src/utils.php

 use InvalidArgumentException;
 use PhpParser\Node;
 use Sabre\Event\{Loop, Promise, EmitterInterface};
+use function Sabre\Uri\parse;


please only alias Sabre\Uri and reference with Uri\parse(), otherwise it is harder to understand what is going on.

felixfbecker · 2017-02-06T21:23:07Z

src/utils.php

+ * @param $b string
+ * @return integer
+ */
+function sortByLeastSlashes($a, $b)


make this function take the array and call usort on it
rename to sortUrisLevelOrder, it makes it more clear what it does

felixfbecker · 2017-02-06T21:25:03Z

src/LanguageServer.php

 use Throwable;
 use Webmozart\PathUtil\Path;
 use Sabre\Uri;
+use function LanguageServer\sortByLeastSlashes;


the function is in the same namespace

felixfbecker

Could you also look for the other places where vendor is assumed?

felixfbecker · 2017-02-06T21:37:10Z

src/utils.php

- * @param $b string
- * @return integer
+ * @param array
+ * @return array


@return void

felixfbecker · 2017-02-06T21:37:36Z

src/utils.php

 {
-    return substr_count(parse($a)['path'], '/') - substr_count(parse($b)['path'], '/');
+    usort($uriList, function ($a, $b) {
+        return substr_count(Uri\parse($a)['path'], '/') - substr_count(Uri\parse($b)['path'], '/');


only parse the URI once

wulftone · 2017-02-06T21:53:25Z

There are a few places where vendor occurs, but I'm at a loss for a good strategy to deal with all of them all. Some are embedded in interesting places and getting a reference to composerJson in there is not straightforward. I'd like to centralize the logic if possible, but if you could give me a bit of architectural direction that would be quite helpful...

wulftone · 2017-02-06T21:56:02Z

src/PhpDocument.php
231:        return strpos($path, '/vendor/') !== false;

src/Server/TextDocument.php
412:            if (preg_match('/\/vendor\/([^\/]+\/[^\/]+)\//', $def->symbolInformation->location->uri, $matches) && $this->composerLock !== null) {

src/Server/Workspace.php
125:                    preg_match('/\/vendor\/([^\/]+\/[^\/]+)\//', $def->symbolInformation->location->uri, $matches);

I think TextDocument will be straightforward but not PhpDocument. Workspace might be doable simply, too. Looking into it now.

felixfbecker · 2017-02-06T21:58:09Z

The isVendored() method can be removed in favor of just checking from the outside on getUri(). The other places shouldn't be too hard to inject an instance of the composer.json file

felixfbecker · 2017-02-06T22:25:53Z

src/utils.php

+    // Parse URIs so we are not continually parsing them while sorting
+    $parsedUriList = array_map(function ($uri) {
+        return Uri\parse($uri);
+    }, $uriList);


I'm sorry, I had a brain fart in my last review. I though $a and $b were the same, but they are not.
This is just micro optimisation, you can git revert it :)

Haha ok I was wondering like, "does he have a use-case with a million files?" 🎈

This reverts commit 8d439f8.

felixfbecker · 2017-02-07T08:24:58Z

src/Index/ProjectIndex.php

    public function getIndexForUri(string $uri): Index
    {
-        if (preg_match('/\/vendor\/([^\/]+\/[^\/]+)\//', $uri, $matches)) {
+        if (\LanguageServer\uriInVendorDir($this->composerJson, $uri, $matches)) {


alias the function

felixfbecker · 2017-02-07T08:26:34Z

src/utils.php

+ * @param array          $matches
+ * @return int
+ */
+function uriInVendorDir(\stdClass $composerJson = null, string $uri, &$matches): int


can you rename this to getPackageName and make it return a string or null?

felixfbecker · 2017-02-07T08:26:48Z

tests/PhpDocumentTest.php

    {
        $document = $this->createDocument('file:///dir/vendor/x.php', "<?php\n$\$a = new SomeClass;");
-        $this->assertEquals(true, $document->isVendored());
+        $this->assertEquals(true, \LanguageServer\isVendored($document));


alias the function

felixfbecker · 2017-02-07T08:27:05Z

src/utils.php

+ */
+function getVendorDir(\stdClass $composerJson = null): string
+{
+    return isset($composerJson->config->{'vendor-dir'}) ?


I think config could be undefined too.

It should work correctly, see the following example:

$ psysh Psy Shell v0.7.1 (PHP 7.1.1 — cli) by Justin Hileman >>> $a = new \stdClass; => {#197} >>> $a->foo->bar = 1 PHP warning: Creating default object from empty value on line 1 >>> $a => {#197 +"foo": {#200 +"bar": 1, }, } >>> isset($a->foo->bar); => true >>> isset($a->foo->baz); => false >>> isset($a->foz->baz); => false // isset operates just like @ and suppresses errors for the whole line (in parens anyway)

felixfbecker

Almost LGTM

felixfbecker · 2017-02-07T21:06:41Z

src/Server/Workspace.php

 use Sabre\Event\Promise;
 use function Sabre\Event\coroutine;
 use function LanguageServer\waitForEvent;
+use function LanguageServer\getPackageName;


use a group use statement

felixfbecker · 2017-02-07T21:08:02Z

src/utils.php

+ * @return string
 */
-function uriInVendorDir(\stdClass $composerJson = null, string $uri, &$matches): int
+function getPackageName(\stdClass $composerJson = null, string $uri): ?string


I am pretty sure the return syntax you are using is only available in PHP7.1. That would be a breaking change, please omit the return type annotation

felixfbecker · 2017-02-07T21:08:16Z

src/utils.php

 * @param string         $uri
 * @param array          $matches
- * @return int
+ * @return string


this should be @return string|null

felixfbecker · 2017-02-07T21:10:19Z

src/utils.php

    $vendorDir = str_replace('/', '\/', getVendorDir($composerJson));
-    return preg_match("/\/$vendorDir\/([^\/]+\/[^\/]+)\//", $uri, $matches);
+    preg_match("/\/$vendorDir\/([^\/]+\/[^\/]+)\//", $uri, $matches);
+    return isset($matches[1]) ? $matches[1] : null;


return $matches[1] ?? null;

felixfbecker · 2017-02-07T21:11:19Z

src/Server/TextDocument.php

    {
        $document = $this->documentLoader->open($textDocument->uri, $textDocument->text);
-        if (!$document->isVendored()) {
+        if (!\LanguageServer\isVendored($document, $this->composerJson)) {


alias the function

felixfbecker · 2017-02-07T21:12:06Z

src/utils.php

+ * @param array          $matches
+ * @return string
+ */
+function getPackageName(\stdClass $composerJson = null, string $uri): ?string


can you change this to take $composerJson as second parameter, so it's consistent with isVendored()?

missed a vendor reference!

wulftone · 2017-02-07T22:00:30Z

Lotta commits in here, might want to squash and merge so it's cleaner (I usually rebase and squash for my own work)

felixfbecker · 2017-02-07T22:20:35Z

Thanks!

wulftone · 2017-02-07T22:21:23Z

Thank you! Happy to collaborate with a responsive maintainer.

Read vendor directory from project's composer.json, if set.

f43e0dc

If not set, default to "vendor". Also sort found vendor directories so we obtain the correct composer.json and composer.lock files

wulftone force-pushed the allow-custom-vendor-dir branch from 9e2a675 to f43e0dc Compare February 6, 2017 04:26

felixfbecker requested changes Feb 6, 2017

View reviewed changes

Trevor Bortins added 2 commits February 6, 2017 13:12

Sort by least number of slashes

81aed92

Change log message

use isset

6146e6c

felixfbecker requested changes Feb 6, 2017

View reviewed changes

felixfbecker reviewed Feb 6, 2017

View reviewed changes

Clean up style

f84b22e

felixfbecker requested changes Feb 6, 2017

View reviewed changes

Minimize URI parsing in loop

8d439f8

felixfbecker reviewed Feb 6, 2017

View reviewed changes

Trevor Bortins added 3 commits February 6, 2017 14:38

Revert "Minimize URI parsing in loop"

0a69c72

This reverts commit 8d439f8.

returns a void

16e27f9

handle other vendor references

682cb2a

felixfbecker requested changes Feb 7, 2017

View reviewed changes

Trevor Bortins and others added 2 commits February 7, 2017 08:21

refactor uriInVendorDir to getPackageName

d95874d

Merge branch 'master' into allow-custom-vendor-dir

4cf3fef

felixfbecker requested changes Feb 7, 2017

View reviewed changes

Trevor Bortins added 2 commits February 7, 2017 13:53

refactor for style, consistency

ed922e1

missed a vendor reference!

forgot the php7.1 syntax thing, removed.

963c5eb

felixfbecker added 2 commits February 7, 2017 23:14

Add space

af00745

Add space

0ad8f34

felixfbecker approved these changes Feb 7, 2017

View reviewed changes

felixfbecker merged commit d5c54ac into felixfbecker:master Feb 7, 2017

wulftone deleted the allow-custom-vendor-dir branch February 7, 2017 22:21

Read vendor directory from project's composer.json, if set. #281

Read vendor directory from project's composer.json, if set. #281

Uh oh!

Conversation

wulftone commented Feb 6, 2017

Uh oh!

codecov bot commented Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

felixfbecker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixfbecker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wulftone commented Feb 6, 2017

Uh oh!

wulftone commented Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felixfbecker commented Feb 6, 2017

Uh oh!

felixfbecker Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixfbecker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Feb 6, 2017 •

edited

Loading

wulftone commented Feb 6, 2017 •

edited

Loading

felixfbecker Feb 6, 2017 •

edited

Loading