In some situations, pass JSON_THROW_ON_ERROR when decoding JSON

Changes in package_manager/src/ComposerInspector.php can be removed once 🐛 \Drupal\package_manager\ComposerInspector::getInstalledPackagesList should use realpath() when checking metapackage path Fixed is in.

Postponed on 🐛 \Drupal\package_manager\ComposerInspector::getInstalledPackagesList should use realpath() when checking metapackage path Fixed

Blocker is merged!

Discussed this with @tedbow, we should check this for all the occurrences of json_decode not only build tests!

Looks good but i wonder what about Json::Decode do we replace all those instance with json_decode and add the flags ?

All instances of Json::Decode are now replaced, everything else looks good already. , nice work.

reviewing

Looks good! thanks!

The repetition here concerns me. Maybe this calls for a utility method on ComposerInspector to always decode JSON the same way. What do you think, @tedbow?

Discussed with @tedbow.

I really don't want to repeat the same call a million times -- in the best case, it's sometimes unnecessary, and in the worst case, it's fragile when spread widely over the codebase.

I've gone through this with a fine-tooth comb and suggested where, IMHO, we should keep the JSON_THROW_ON_ERROR flag, where we should revert it, and where we should replace it with something else. Leaving assigned to @tedbow to implement these changes, since he may have different ideas than me about what should be changed.

@phenaproxima mostly accepted your suggestions. Commented on the others in the MR

Generally I'm okay with your decisions, but there are a couple of places where I don't...thoughts?

We need more of summary for this to be reviewable otherwise we can't know if is correct

@phenaproxima since you made the comment the quoted in the original issue to add one instance of `JSON_THROW_ON_ERROR` and then this issue started to add them everywhere and then we removed them again could you update the summary to what the goal and proposed solutions is here

Okay, I updated the summary. Assigning back to @tedbow to respond to my feedback.

Discussed with @tedbow on Zoom.

I really, really don't like the repetition, and the need to always pass the recursion depth, but Ted correctly pointed out that adding a protected static method on TemplateProjectTestBase would, by core's policies, be considered API. Even if we explicitly marked the base class internal.

Neither of us likes that policy one bit. The concept of "internal" is rendered utterly pointless if we don't stand by it in practice. However, this is a core policy that we have to abide by, at least for now, until it changes in a more sane direction.

But, then I found a compromise: we can take advantage of PHP 8's support for named arguments, and leave out the recursion depth. So this:

json_decode($value, TRUE, 512, JSON_THROW_ON_ERROR);

Becomes this:

json_decode($value, TRUE, flags: JSON_THROW_ON_ERROR);

This is a compromise I can live with. True, we'd be repeating the call several times because of core's dumb policy, but we also wouldn't need to pass a worthless and confusing extra argument.

implemented #33

Looks good to when tests pass.

The use of named parameters thanks to PHP 8 makes sense. 👍

The inconsistent updating in this commit of some json_decode() calls to Json::decode() instead, whereas in other cases doing the exact opposite makes no sense to me. I see there's been discussion about this, but I don't see a conclusion documented anywhere and I cannot discern any pattern.

For example:

• package_manager/src/Validator/AllowedScaffoldPackagesValidator.php was updated to START using Json::decode()
• package_manager/src/Validator/AllowedScaffoldPackagesValidator.php was updated to STOP using Json::decode()

It appears that the pattern is to use Json::decode() in all non-test cases except in cases where we want to set the JSON_THROW_ON_ERROR flag? But why do we want that flag in the one case but not in the other?

That information is missing from the issue's comments.