Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default packages – 6-beta-2 bugfix #49

Merged
merged 4 commits into from
Jun 2, 2020

Conversation

BrianHenryIE
Copy link
Contributor

@BrianHenryIE BrianHenryIE commented May 28, 2020

The previously merged default-packages PR #34 didn't work in the beta.

When that didn't work, I noticed couple of things: the parsing of the config didn't output a single line "missing packages key" and then the attempt to use the null object caused further verbose error messages.

Now it defaults to an empty array, prefers composer->extra->mozart->packages and falls back to composer->require if available.

The unit tests I wrote are only to check composer.json exists, is valid json, and has the extra->mozart keys as objects. It wasn't until I opened the PR I see how I could've put in a mock Replacer to fully test the changes. I should read a book on PHP!

@BrianHenryIE
Copy link
Contributor Author

I returned 1 on error, which I think is adequate, since the superclass says "0 if everything went fine, or an exit code".

@BrianHenryIE BrianHenryIE changed the title Default packages – beta bugfix Default packages – 6-beta-2 bugfix May 28, 2020
@coenjacobs
Copy link
Owner

This looks solid. Great that you’ve added tests for the verification logic as well. Will test this soon, to see how it gets along with my work in #47, but this should go into 0.6 anyway. Thanks!

@coenjacobs coenjacobs merged commit 965d698 into coenjacobs:master Jun 2, 2020
@BrianHenryIE BrianHenryIE deleted the default-packages-2 branch May 3, 2021 02:22
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.

2 participants