Updated NpmRepository to skip broken versions #308
Conversation
| $packages[] = $loader->load($config); | ||
| } catch (\UnexpectedValueException $exception) { | ||
| // Most probably version constraint is broken. | ||
| // Skip this version and hope that another one will be OK |
There was a problem hiding this comment.
There should be a warning or notice, I think.
There was a problem hiding this comment.
Good point, done
Repository/NpmRepository.php
Outdated
| $this->io->write("<warning>Skipped {$config['name']} version {$version}: {$exception->getMessage()}</warning>", IOInterface::VERBOSE); | ||
| continue; | ||
| } catch (\Exception $exception) { | ||
| throw $exception; |
There was a problem hiding this comment.
why catch it if you don't do anything with it?
There was a problem hiding this comment.
@SilverFire See my previous review, because it is past in outdated status with your new update, but it's not the case: this catch is unnecessary.
There was a problem hiding this comment.
I also think this can be removed, you're just catching and throwing the exception again :)
There was a problem hiding this comment.
Opps. It used id for debug to place breakpoint on throw $exception, then I just forgot to remove it. Thank you for pointing out, friends :)
Repository/NpmRepository.php
Outdated
| // Most probably version constraint is broken. | ||
| // Skip this version and hope that another one will be OK | ||
| continue; | ||
| } catch (\Exception $exception) { |
There was a problem hiding this comment.
This catch is unnecessary.
| // Most probably version constraint is broken. | ||
| // Skip this version and hope that another one will be OK | ||
| $this->io->write("<warning>Skipped {$config['name']} version {$version}: {$exception->getMessage()}</warning>", IOInterface::VERBOSE); | ||
| continue; |
There was a problem hiding this comment.
continueis not necessary- remove comments because the output message is explicit
And it will be good for me.
There was a problem hiding this comment.
remove comments because the output message is explicit
the message says much less than the comment. should probably improve the message to say, why the version was skipped.
There was a problem hiding this comment.
$exception->getMessage() will include the exact reason, e.g. "Could not parse version constraint >=1.1.0,,<2.0: Invalid version string "1.1.0,,<2.0"
There was a problem hiding this comment.
I'm for keeping the comment then, it does not hurt and provides more context.
|
@SilverFire Thank you! |
|
Thank you too |
Related to hiqdev/asset-packagist#68
Some packages have dependencies with corrupted version constraints.
The change allows composer-asset-plugin to omit these versions.