fxpio / composer-asset-plugin

NPM/Bower Dependency Manager for Composer
MIT License
892 stars 156 forks source link

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

Open sakhunzai opened 9 years ago

sakhunzai commented 9 years ago

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 commented 9 years ago

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

sakhunzai commented 9 years ago

yes both are latest :

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

francoispluchino commented 9 years ago

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

sakhunzai commented 9 years ago

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 commented 9 years ago

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