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

fix custom composer vendor-dir loading #66

Merged
merged 8 commits into from Dec 14, 2017

Conversation

3 participants
@o0h
Contributor

o0h commented Dec 14, 2017

Purpose

With composer config "vendor-dir", AbstractAnnotator fail to manual autoload for php_codesniffer.
To fix this, find composer package directory path in ancestor instead of 'getcwd() . '/vendor/' .

What happened

in composer.json

{
    "config": {
        "vendor-dir": "vendor/php-composer"
    }
}

run bin/cake IdeHelper.annotations all to throw exception.

bash-4.2# bin/cake  IdeHelper.annotations all
[Models]
Warning Error: Class '\PHP_CodeSniffer_File' not found in [/var/www/html/vendor/composer/dereuromark/cakephp-ide-helper/src/Annotator/AbstractAnnotator.php, line 29]

2017-12-14 11:34:47 Warning: Warning (2): Class '\PHP_CodeSniffer_File' not found in [/var/www/html/vendor/composer/dereuromark/cakephp-ide-helper/src/Annotator/AbstractAnnotator.php, line 29]
Trace:
Cake\Error\BaseErrorHandler::handleError() - CORE/src/Error/BaseErrorHandler.php, line 153
class_alias - [internal], line ??
include - ROOT/vendor/composer/dereuromark/cakephp-ide-helper/src/Annotator/AbstractAnnotator.php, line 29
Composer\Autoload\includeFile - ROOT/vendor/composer/composer/ClassLoader.php, line 444
Composer\Autoload\ClassLoader::loadClass() - ROOT/vendor/composer/composer/ClassLoader.php, line 322

What I did

As vendor-dirpath, set two level upper from plugin directory.

@@ -21,7 +21,10 @@
use RuntimeException;
use SebastianBergmann\Diff\Differ;
$manualAutoload = getcwd() . '/vendor/squizlabs/php_codesniffer/autoload.php';
$currentDir = __DIR__;
$vendorDir = substr($currentDir, 0, strpos($currentDir, '/cakephp-ide-helper'));

This comment has been minimized.

@dereuromark

dereuromark Dec 14, 2017

Owner

Could this fail on windows? maybe use DS here when you compare strings.

@codecov-io

This comment has been minimized.

codecov-io commented Dec 14, 2017

Codecov Report

Merging #66 into master will decrease coverage by 0.09%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master      #66     +/-   ##
===========================================
- Coverage     89.93%   89.84%   -0.1%     
  Complexity      605      605             
===========================================
  Files            33       33             
  Lines          1649     1654      +5     
===========================================
+ Hits           1483     1486      +3     
- Misses          166      168      +2
Impacted Files Coverage Δ Complexity Δ
src/Annotator/AbstractAnnotator.php 89.56% <66.66%> (-0.55%) 101 <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 12eb57d...3e7883c. Read the comment docs.

@dereuromark

This comment has been minimized.

Owner

dereuromark commented Dec 14, 2017

Can you please check if the change works for you? Hopefully this makes the tests pass again.

@o0h

This comment has been minimized.

Contributor

o0h commented Dec 14, 2017

case: vendor/ directory exists and php composer vendor-dir is set to vendor/php-composer , $manualAutoload will be invalid path 😢

  • expect vendor/php-composer/squizlabs/php_sniffer/autoload.php
  • actual vendor/squizlabs/php_codesniffer/autoload.php

(my team also use ruby bundler, so there are vendor/bundle and vendor/php-composer.

@dereuromark

This comment has been minimized.

Owner

dereuromark commented Dec 14, 2017

Can u reverse the lookup order?

o0h added some commits Dec 14, 2017

@o0h

This comment has been minimized.

Contributor

o0h commented Dec 14, 2017

i tried to fix #66 (comment)

@dereuromark dereuromark merged commit a5dfdae into dereuromark:master Dec 14, 2017

1 of 3 checks passed

codecov/patch 66.66% of diff hit (target 89.93%)
Details
codecov/project 89.84% (-0.1%) compared to 12eb57d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment