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

Survive through a malformed installed.json file. #391

Merged
merged 3 commits into from
Jul 30, 2022

Conversation

bucefal91
Copy link
Contributor

We have been using Kint and occasionally would run into the following concern.

Somehow, during the deployments (when the system is transitioning from "old" to the "new" state), we would encounter PHP warnings

PHP Warning:  Invalid argument supplied for foreach() in ....../vendor/kint-php/kint/src/Utils.php on line 87

I am unable to reproduce it "per se". But any simple change to the vendor/composer/installed.json will reproduce it, i.e. just edit this file and make it a "malformed" JSON. The point is that \json_decode(\file_get_contents($installed), true); would fail to parse it. Technically, composer could fail or people could manually tamper the file so I think "dealing" with the edge case where the file does not contain valid JSON is a worthwhile thing to have :)

With this patch/PR applied, even if you "mess up" your vendor/composer/installed.json, the warning will not be generated.

@jnvsor
Copy link
Member

jnvsor commented Jul 26, 2022

Hi @bucefal91 - nice catch!

The phar will need to be rebuilt, and I'd like an additional test case for this edge case too. Do you want to try your hand at it or shall I?

@bucefal91
Copy link
Contributor Author

Hello, @jnvsor ,

Thank you for your prompt reply. I sure can try :) I hope to have it done by the end of today.

@jnvsor
Copy link
Member

jnvsor commented Jul 26, 2022

You should just be able to add a file_put_contents and assert at the end of UtilsTest::testComposerGetExtras and that'll do the trick

@bucefal91
Copy link
Contributor Author

@jnvsor ,

I think I have applied your proposals/feedback:

  • .phar file "built" & committed.
  • I figured I missed a code style thing in the Utils class.
  • Added the test coverage into UtilsTest::testComposerGetExtras() and I confirmed the amended test passes in this branch and fails in master.

@jnvsor
Copy link
Member

jnvsor commented Jul 29, 2022

Hi @bucefal91 sorry for the delay I didn't get a notification from gh

Looks good and I presume the tests will pass too (Looks like the build didn't match, did you format after build or before?)

Could you use assertSame([], ... instead of assertEmpty in the test? That way we can check the type too (It's essentially === vs empty())

@bucefal91
Copy link
Contributor Author

@jnvsor ,

Yes, sir! :) I've changed it to the strict equality in the test and rebuilt the .phar file anew.

@jnvsor jnvsor merged commit 84a61c9 into kint-php:master Jul 30, 2022
@jnvsor
Copy link
Member

jnvsor commented Jul 30, 2022

Passed, merged, tagged, pushed. Good job!

@bucefal91 bucefal91 deleted the feature/malformed-installed-json branch August 1, 2022 14:42
@bucefal91
Copy link
Contributor Author

Thank you, I appreciate your support and desire to take a patch into the project. :)

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