Skip to content

Package path declaration#52

Closed
sbuzonas wants to merge 6 commits into
composer:masterfrom
fancyguy:develop
Closed

Package path declaration#52
sbuzonas wants to merge 6 commits into
composer:masterfrom
fancyguy:develop

Conversation

@sbuzonas

Copy link
Copy Markdown

This may be a better approach to what I was trying to achieve in my WordPress package pull request. Certain cases may be flexible and allow names/locations to be changed. This patch in my case for WordPress allows a custom content directory to be declared, as well as allows the composer root to live outside of the wordpress directory.

{
    "extra": {
        "package-paths": {
            "wordpress/wp-content/plugins/{$name}/": ["wordpress-plugin"],
            "wordpress/wp-content/themes/{$name}/": ["wordpress-theme"]
        }
    }
}

Can install all wordpress plugins and themes into the wordpress directory where the actual installation lives, opposed to maintaining an array of each of the package types in the install-paths array along side of the regular requirement declarations.

This time not limited to only wordpress.

@shama

shama commented Dec 27, 2012

Copy link
Copy Markdown
Contributor

I appreciate your pull request but the consensus on this feature is installers should not support this. The reason is it enables a package author to wipe out folders on a user's system, in a potentially devastating fashion. Most likely without the user ever knowing. This is why only the user has the ability to override the path within their composer.json and not a package author.

I've recently added a note about this to the readme as it's a commonly requested feature (and was once implemented then removed): https://github.com/composer/installers#should-we-allow-dynamic-package-types-or-paths-no

Please reference these issues for precedence: #41, #22, #21, #20, and #10.

@shama shama closed this Dec 27, 2012
@sbuzonas

Copy link
Copy Markdown
Author

What is different about extra.installer-paths?

@sbuzonas

Copy link
Copy Markdown
Author

I thought that:

$this->composer->getPackage()

Returned the root package, and obtaining the extra from the root package would be the end user's decision to install a package at an arbitrary location.

@shama

shama commented Dec 27, 2012

Copy link
Copy Markdown
Contributor

Oh I thought you we're asking to implement custom install paths from the package author end. I looked at your PR closer and realized it's an alias for installer-paths. Why do we need two extra names for the same thing?

@sbuzonas

Copy link
Copy Markdown
Author

The extra.package-paths changes the path location on a package type basis. I imagine in a number of scenarios if one package type needs a custom install path all of them do. This prevents necessity to update every package pulled in by composer to be listed in an array for a specific type.

I have a wordpress install configured with a custom content directory outside of the wordpress directory. This keeps wordpress clean and directly from the upstream VCS, and my satis repositories have all of my plugins and themes configured to utilize composer/installers. This gives me an end result of installing them in wp-content/my_plugin which wp-content is a sibling to wordpress. This patch allows for the package-type install path to be overridden in the root composer.json to set the install location to wordpress/wp-content or in my case omni-content without listing all 100+ plugins/themes in the arrays for installer-paths.

@sbuzonas

Copy link
Copy Markdown
Author

I also have a number of cake applications that can benefit from this because they have a shared plugin directory outside of the application.

@shama

shama commented Dec 28, 2012

Copy link
Copy Markdown
Contributor

Oh I see, this allows a consumer to customize the path based on type as opposed to just the vendor/name. I'll reopen it. I think we should come up with a different key for the extra though. Or maybe implement a convention in installer-paths to signal to match a different key, such as:

"installer-paths": {
  "wordpress/wp-content/plugins/{$name}/": ["type!wordpress-plugin"],
}

Or something. If supporting type as a key, it would be nice to support other keys as well without adding a bunch of new extras. Another idea similar is to allow regex for matching packages as suggested here: #21 (comment)

@Seldaek what is your opinion on being able to configure installer-paths to match other keys?

@shama shama reopened this Dec 28, 2012
@shama

shama commented Dec 28, 2012

Copy link
Copy Markdown
Contributor

Or maybe it's safe to assume if the value within the array doesn't have a / in them... assume it's a type:
"wordpress/wp-content/plugins/{$name}/": ["wordpress-plugin"] would match by type and
"wordpress/wp-content/plugins/{$name}/": ["vendor/wordpress-plugin"] would match as it does currently.

@sbuzonas

Copy link
Copy Markdown
Author

I think at the moment assuming that the lack of a / would work for most cases, but not for packages without a vendor.

Perhaps utilizing Installer::findFrameworkType() in some fashion on the passed parameter if there is no /?

@sbuzonas

Copy link
Copy Markdown
Author

Regex could be interesting. In the WordPress scenario:

"installer-paths": {
    "wordpress/wp-content/${1}/{$name}/": ["^wordpress-(\w+)"]
}

Maybe to maintain backwards compatibility, and utilize the same extra array... If BaseInstaller::mapCustomInstallPaths() comes back false sending the package name, prepend the package name with the package type affixed with something like a :. Then it can go through a regex match/replace type of function.

@shama

shama commented Dec 29, 2012

Copy link
Copy Markdown
Contributor

All package names must have a vendor. Trying to keep it simple to avoid a bajillion support issues from opening. The regex looks too messy and problematic; let's scratch that idea.

@sbuzonas

Copy link
Copy Markdown
Author

If all packages must have a vendor name, then the first solution would work assuming that items with a / are packages, and the remainder are types. I was basing my previous statement on existing test cases in the suite.

@shama

shama commented Dec 30, 2012

Copy link
Copy Markdown
Contributor

Oh thanks for pointing that out. I wrote that test but it might have been a mistake, #25. I'll ask around when people are back around from holiday.

@Seldaek

Seldaek commented Jan 4, 2013

Copy link
Copy Markdown
Member

If you must, allowing "type:foo" sounds like the less insane to me. Please do not rely on the slash being there. That's just enforced on packagist, but is not a composer predicament (php is a valid package, ext-xml too).

@shama

shama commented Jan 4, 2013

Copy link
Copy Markdown
Contributor

My mistake, thank you Jordi. @slbmeh feel free to open a new PR or update this one for installer-paths detecting types if you're still interested in this. Thanks!

"installer-paths": {
  "wordpress/wp-content/plugins/{$name}/": ["type:wordpress-plugin"],
}

@chrisvanpatten

Copy link
Copy Markdown

I also use an alternate WordPress path and would love to see this implemented! Thanks for writing it up.

@shama

shama commented May 23, 2013

Copy link
Copy Markdown
Contributor

Merged. Thanks Steve! Sorry it took so long.

@sbuzonas

Copy link
Copy Markdown
Author

No problem, thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants