Verify package name(s) when using an update whitelist #1112

Closed
wants to merge 12 commits into
from

Conversation

Projects
None yet
5 participants
@thewilkybarkid
Contributor

thewilkybarkid commented Sep 15, 2012

I found out about the composer update nothing command/hack at Symfony Live London. This seems like it is occasionally useful, but is only possible as the package name list isn't verified. This means that misspelling a package name would have the same effect, without the user necessarily realising that they've made a mistake.

This verifies the requested name(s) against the list of packages, returning an error when one isn't found

So:

php composer.phar update monolog/monolo

Would result in:

Package monolog/monolo not known

The nothing hack is still permitted (as long as it's on its own, not in a list of package names).

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Sep 16, 2012

Contributor

I think this forbids adding a new dependency in your project and doing a partial update to get it without updating the other dependencies, as the new package will not be available in the local repository.
And this case is the most common one where a partial update is needed IMO

Contributor

stof commented Sep 16, 2012

I think this forbids adding a new dependency in your project and doing a partial update to get it without updating the other dependencies, as the new package will not be available in the local repository.
And this case is the most common one where a partial update is needed IMO

@thewilkybarkid

This comment has been minimized.

Show comment
Hide comment
@thewilkybarkid

thewilkybarkid Sep 16, 2012

Contributor

That is now be fixed (along with dev packages) so you can add, update and remove individual packages.

Contributor

thewilkybarkid commented Sep 16, 2012

That is now be fixed (along with dev packages) so you can add, update and remove individual packages.

@Seldaek

View changes

src/Composer/Installer.php
+ if (count($packages) > 1 || $packages[0] !== 'nothing') {
+ $knownPackages = array();
+ foreach ($this->repositoryManager->getLocalRepository()->getPackages() as $localPackage) {
+ $knownPackages[] = strtolower($localPackage->getName());

This comment has been minimized.

@Seldaek

Seldaek Sep 16, 2012

Member

To be perfectly safe I think this should be getNames and not getName, that way if a package is installed because it provides something you require, it will still match .

@Seldaek

Seldaek Sep 16, 2012

Member

To be perfectly safe I think this should be getNames and not getName, that way if a package is installed because it provides something you require, it will still match .

This comment has been minimized.

@thewilkybarkid

thewilkybarkid Sep 16, 2012

Contributor

Ah, hadn't spotted that. Just walked through the code and realised strtolower() is redundant too.

@thewilkybarkid

thewilkybarkid Sep 16, 2012

Contributor

Ah, hadn't spotted that. Just walked through the code and realised strtolower() is redundant too.

@Seldaek

View changes

src/Composer/Command/UpdateCommand.php
+ ->setUpdateWhitelist($input->getArgument('packages'))
+ ;
+ } catch (\Exception $e) {
+ $io->write('<error>' . $e->getMessage() . '</error>');

This comment has been minimized.

@Seldaek

Seldaek Sep 16, 2012

Member

This isn't a great idea, if you throw a very specific exception and catch that ok, but as such it will basically prevent us from seeing the backtrace when a real exception occurs.

@Seldaek

Seldaek Sep 16, 2012

Member

This isn't a great idea, if you throw a very specific exception and catch that ok, but as such it will basically prevent us from seeing the backtrace when a real exception occurs.

This comment has been minimized.

@thewilkybarkid

thewilkybarkid Sep 16, 2012

Contributor

Fair point; there aren't any Composer exceptions yet, ok to create something like Composer\Exception\UnknownPackageNameException which extends \UnexpectedValueException? Couldn't see a non-exception method to halt the flow here (without rather ugly refactoring).

@thewilkybarkid

thewilkybarkid Sep 16, 2012

Contributor

Fair point; there aren't any Composer exceptions yet, ok to create something like Composer\Exception\UnknownPackageNameException which extends \UnexpectedValueException? Couldn't see a non-exception method to halt the flow here (without rather ugly refactoring).

This comment has been minimized.

@Seldaek

Seldaek Sep 16, 2012

Member

Yeah that's alright (the new exception)

@Seldaek

Seldaek Sep 16, 2012

Member

Yeah that's alright (the new exception)

@stof

View changes

src/Composer/Command/UpdateCommand.php
+ } catch (UnknownPackageException $e) {
+ $io->write('<error>' . $e->getMessage() . '</error>');
+
+ return false;

This comment has been minimized.

@stof

stof Sep 16, 2012

Contributor

A command should return an integer (the exit code), not a boolean.

@stof

stof Sep 16, 2012

Contributor

A command should return an integer (the exit code), not a boolean.

@stof

View changes

src/Composer/Installer.php
*/
public function setUpdateWhitelist(array $packages)
{
+ if (count($packages) == 0) {

This comment has been minimized.

@stof

stof Sep 16, 2012

Contributor

Please use a strict comparison

@stof

stof Sep 16, 2012

Contributor

Please use a strict comparison

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Sep 16, 2012

Contributor

You should add some tests for this.

Contributor

stof commented Sep 16, 2012

You should add some tests for this.

@thewilkybarkid

This comment has been minimized.

Show comment
Hide comment
@thewilkybarkid

thewilkybarkid Sep 18, 2012

Contributor

I've been able to create a test that passes (first of a few), can I just check that it's how you would expect it?

I've created a Fixtures/installer/update-whitelist-unknown-package.test file which contains:

[...]
--RUN--
update not/known
--EXPECT-OUTPUT--
<error>Package not/known not known</error>

--EXPECT--

Then inside InstallerTest::testIntegration() I have to catch the exception and output the error:

// line 203
$application->get('update')->setCode(
    function ($input, $output) use ($installer, $io) {
        try {
            $installer
                ->setDevMode($input->getOption('dev'))
                ->setUpdate(true)
                ->setUpdateWhitelist($input->getArgument('packages'));

            return $installer->run() ? 0 : 1;
        } catch (\Composer\Exception\UnknownPackageException $e) {
            $io->write('<error>' . $e->getMessage() . '</error>');

            return 0;
        }
    }
);

I've had to return 0 rather than the usual 1 to pass one of the assertions. Could --EXPECT-EXIT-CODE-- (which defaults to 0) be added like in the functional tests? That would make expected-error testing a little nicer.

Contributor

thewilkybarkid commented Sep 18, 2012

I've been able to create a test that passes (first of a few), can I just check that it's how you would expect it?

I've created a Fixtures/installer/update-whitelist-unknown-package.test file which contains:

[...]
--RUN--
update not/known
--EXPECT-OUTPUT--
<error>Package not/known not known</error>

--EXPECT--

Then inside InstallerTest::testIntegration() I have to catch the exception and output the error:

// line 203
$application->get('update')->setCode(
    function ($input, $output) use ($installer, $io) {
        try {
            $installer
                ->setDevMode($input->getOption('dev'))
                ->setUpdate(true)
                ->setUpdateWhitelist($input->getArgument('packages'));

            return $installer->run() ? 0 : 1;
        } catch (\Composer\Exception\UnknownPackageException $e) {
            $io->write('<error>' . $e->getMessage() . '</error>');

            return 0;
        }
    }
);

I've had to return 0 rather than the usual 1 to pass one of the assertions. Could --EXPECT-EXIT-CODE-- (which defaults to 0) be added like in the functional tests? That would make expected-error testing a little nicer.

@pborreli

View changes

src/Composer/Installer.php
+ }
+ }
+ }
+
$this->updateWhitelist = array_flip(array_map('strtolower', $packages));

This comment has been minimized.

@pborreli

pborreli Sep 18, 2012

Contributor

is it possible to make the array_map('strtolower', $packages)before line 754 so you can remove the strtolower($package)on line 772 ?

@pborreli

pborreli Sep 18, 2012

Contributor

is it possible to make the array_map('strtolower', $packages)before line 754 so you can remove the strtolower($package)on line 772 ?

This comment has been minimized.

@thewilkybarkid

thewilkybarkid Sep 18, 2012

Contributor

Only downside is that it would lose the case for the error message: I think such messages should ideally contain exactly what you typed, which includes the case used. (Though I'm not that fussed about it!)

@thewilkybarkid

thewilkybarkid Sep 18, 2012

Contributor

Only downside is that it would lose the case for the error message: I think such messages should ideally contain exactly what you typed, which includes the case used. (Though I'm not that fussed about it!)

This comment has been minimized.

@pborreli

pborreli Sep 18, 2012

Contributor

well i was thinking more like that :

$lowercasedPackages = array_map('strtolower', $packages);

then

foreach ($lowercasedPackages as $key => $package) {
    if (!in_array($package, $knownPackages)) {
        throw new UnknownPackageException('Package ' . $packages[$key] . ' not known');
    }   
}

it might look like a micro-optimization but i always see in callgraph that strtolower is often too much called

@pborreli

pborreli Sep 18, 2012

Contributor

well i was thinking more like that :

$lowercasedPackages = array_map('strtolower', $packages);

then

foreach ($lowercasedPackages as $key => $package) {
    if (!in_array($package, $knownPackages)) {
        throw new UnknownPackageException('Package ' . $packages[$key] . ' not known');
    }   
}

it might look like a micro-optimization but i always see in callgraph that strtolower is often too much called

@thewilkybarkid

This comment has been minimized.

Show comment
Hide comment
@thewilkybarkid

thewilkybarkid Oct 4, 2012

Contributor

Does that test style look ok, or you would prefer it a different way?

Contributor

thewilkybarkid commented Oct 4, 2012

Does that test style look ok, or you would prefer it a different way?

*/
public function setUpdateWhitelist(array $packages)
{
- $this->updateWhitelist = array_flip(array_map('strtolower', $packages));
+ if (count($packages) === 0) {

This comment has been minimized.

@stloyd

stloyd Oct 4, 2012

Contributor
if (count($packages) === 0 || (isset($packages[0]) && strtolower($packages[0]) === 'nothing')) {

With such you could remove the if below as well, as the array_map() and move strtolower() into just last loop (with change of $lowercasePackages to $packages).

@stloyd

stloyd Oct 4, 2012

Contributor
if (count($packages) === 0 || (isset($packages[0]) && strtolower($packages[0]) === 'nothing')) {

With such you could remove the if below as well, as the array_map() and move strtolower() into just last loop (with change of $lowercasePackages to $packages).

This comment has been minimized.

@stof

stof Oct 4, 2012

Contributor

@stloyd changing only the condition here would not be equivalent

@stof

stof Oct 4, 2012

Contributor

@stloyd changing only the condition here would not be equivalent

@stof

View changes

src/Composer/Installer.php
*/
public function setUpdateWhitelist(array $packages)
{
- $this->updateWhitelist = array_flip(array_map('strtolower', $packages));
+ if (count($packages) === 0) {
+ return $this;

This comment has been minimized.

@stof

stof Oct 4, 2012

Contributor

this is not equivalent to the previous code. You should reset the update whitelist to an empty array here, otherwise resetting it is not possible anymore

@stof

stof Oct 4, 2012

Contributor

this is not equivalent to the previous code. You should reset the update whitelist to an empty array here, otherwise resetting it is not possible anymore

+*
+* For the full copyright and license information, please view the LICENSE
+* file that was distributed with this source code.
+*/

This comment has been minimized.

@stof

stof Jan 23, 2013

Contributor

Wrong indentation here (the * should be aligned with the opening one)

@stof

stof Jan 23, 2013

Contributor

Wrong indentation here (the * should be aligned with the opening one)

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