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

fails to delete ignore files if asset-installer-paths is defined #131

Open
sakhunzai opened this issue Jul 2, 2015 · 5 comments
Open

fails to delete ignore files if asset-installer-paths is defined #131

sakhunzai opened this issue Jul 2, 2015 · 5 comments
Labels

Comments

@sakhunzai
Copy link

Let say I have following composer.json

{
  "config": {
    "vendor-dir": "assets/vendors"
  },
  "require" : {
    "bower-asset/jquery": "2.1.x",
    "bower-asset/jqueryui": "1.11.x",
    "npm-asset/jquery-json" : "2.5.*"
  },
  "extra": {
    "asset-installer-paths": {
      "npm-asset-library": "assets/components",
      "bower-asset-library": "assets/components"
    },
    "asset-repositories": [],
    "asset-ignore-files": {
      "bower-asset/jquery": [
        "src/.*"
      ]
    }
  }
}

I have defined both vendor-dir as well as asset-installer-paths , given that , composer fails to delete ignore files defined in asset-ignore-files list.

problem seems to be Installer/IgnoreFactory.php class which does not account for the both directory settings, to be specific getInstallDir

 protected static function getInstallDir(Composer $composer, PackageInterface $package, $installDir = null)
    {
        if (null === $installDir) {
            $installDir = rtrim($composer->getConfig()->get('vendor-dir'), '/').'/'.$package->getName();
        }
        return rtrim($installDir, '/');
    }

I think it makes sense to have different paths for vendor-dir and asset-installer-paths , as I dont want to put vendor-dir on web accessible path.

thanks

@francoispluchino
Copy link
Member

Use you the last stable versions (v1.0.2) of this plugin and composer?

@sakhunzai
Copy link
Author

yes both are latest :

composer self-update
composer global require "fxp/composer-asset-plugin:~1.0.2"

@francoispluchino
Copy link
Member

Indeed, it seems that the change of directory is not supported by the IgnoreFactory.

@francoispluchino francoispluchino added this to the 1.0.3 milestone Jul 2, 2015
@sakhunzai
Copy link
Author

some other observation that might help fix the bug

a) A quick fix copying logic from AssetInstaller constructor to Installer\IngnoreFActory::getInstallDir

 protected static function getInstallDir(Composer $composer, PackageInterface $package, $installDir = null)
    {        
        if (null === $installDir) {
            $extra = $composer->getPackage()->getExtra();
            if (!empty($extra['asset-installer-paths'][$this->type])) {
                $vendorDir = rtrim($extra['asset-installer-paths'][$this->type], '/');
            } else {
                $vendorDir = rtrim($this->vendorDir.'/'.$assetType->getComposerVendorName(), '/');
            }
            $installDir = $vendorDir.'/'.$package->getName();
        }

        return rtrim($installDir, '/');
    }

b) A possible bug for future in IgnoreFactory:create() Ln 44

if ($packageName === $package->getName()) {
                    static::addPatterns($manager, $patterns);
                    break;
                }

above code fails for

{ 
  "require":{
      "bower-asset/jquery-ajaxQueue"
    },
 "asset-ignore-files": {
      "bower-asset/jquery-ajaxQueue": []
  }
 }   

because :$package->getName() returns bower-asset/jquery-ajaxqueue while converting all characters to small case, I am not sure if that is part of composer standard.

@francoispluchino
Copy link
Member

Alas, yes, it's the standard of Composer, and that's a problem ...

@francoispluchino francoispluchino removed this from the 1.0.3 milestone Jul 28, 2015
@francoispluchino francoispluchino added 1.x and removed 1.x labels Sep 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants