Only indicate support for package types that are actually supported #99

Closed
wants to merge 5 commits into
from

Projects

None yet

3 participants

Contributor

This pull request is a fix for composer/installers#98

It adds a protected method to Composer\Installers\Installer that checks the installer class for a class constant (PATTERN). The constant is defined already on BaseInstaller, so it will never be undefined. If no pattern is available, it will return the current value of '(\w+)'. Otherwise, it will return a pattern (e.g. for WordPressInstaller, the pattern is '(plugin|theme|muplugin)'. This pattern is used to match the second half of the package type after the framework's slug.

Unit tests have also been updated to reflect the new functionality.

johnpbloch added some commits Aug 16, 2013
@johnpbloch johnpbloch Add a test case to verify that unsupported types do not register as s…
…upported.
83c3270
@johnpbloch johnpbloch Update supports test data to use types that are actually supported 0656108
@johnpbloch johnpbloch Restrict support to actually supported types
This adds a class constant to the BaseInstaller class and all children of the class by which a pattern for matching supported types of packages can be defined. Installer now grabs the value of this constant and uses it in the regular expression check for whether the package type is actually supported after all.
d3bac95
Contributor
shama commented Aug 16, 2013

Thanks for this! I think we should just expose protected $locations to public $locations and use array_keys to retrieve the types. That will keep it a bit more DRY. What do you think?

Contributor

That would be fine. There's a problem with doing that and two ways to fix it (the problem being that we'd be accessing a non-static property statically):

  1. We would have to use public static $locations and access it with static::$locations to avoid an error from trying to access a non-static property statically. At the time we run supports(), we do not have access to the package object, so we can't instantiate the installer.
  2. We would want to make BaseInstaller's constructor arguments optional so we can instantiate installers without a package. This approach would even let us keep locations protected and use a getter method to return them and would require no changes to any child classes.
Contributor

I'd personally be more in favor of number 2 above than of number 1.

Contributor
shama commented Aug 16, 2013

Number 2 sounds good to me as well. Thanks!

johnpbloch added some commits Aug 16, 2013
@johnpbloch johnpbloch Revert "Restrict support to actually supported types"
This reverts commit d3bac95.
29202c5
@johnpbloch johnpbloch Implement location pattern compilation
As discussed on Github, this modifies the BaseInstaller class to make its constructor arguments optional and adds a getter method to get the locations. This allows us to re-use the locations property.
e2d3233
Contributor

bump

So... is this good to go? :D

@shama shama closed this in 49c8a5d Aug 19, 2013
Contributor
shama commented Aug 19, 2013

Didn't realize you updated the PR. Thanks for the fix!

Rarst commented Aug 29, 2013

@shama could you please tag stable version after this fix? I want to update my guide on topic ( http://composer.rarst.net/ ) to using Jonh's installer. Thanks in advance!

Contributor
shama commented Aug 29, 2013

@Rarst Published as 1.0.6.

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