Json and PHP serializers should throw excepiton on failure

anandhi karnan → made their first commit to this issue’s fork.

I have updated the serializers to handle failure scenarios more robustly:

PHP Serializer (PhpSerialize):

• Added exception handling for unserialization errors.
• Incorporated the allowed_classes option to enhance security and prevent unserialization of objects.

JSON Serializer (Json):

• Implemented exception handling for JSON encoding and decoding, including handling invalid JSON and circular references.

Testing:

• Added tests to verify correct behavior for both valid and invalid inputs, covering various edge cases.

This broke all the tests it seems.

Also made validation go down so that should be looked at.

Removing ['allowed_classes' => FALSE] fixed the majority of the failures.

On a cursory glance many of the remaining failures appear to be test design failures in JSON API related tests:

Json::decode((string) $response->getBody()); Appears to be called in several places where getBody() may be an empty string (which is NOT a valid JSON string).

ResourceTestBase::assertResourceResponse() (a basic helper method) appears to trigger this as it will attempt to cast the response body as a decoding string to be used in the message without any knowledge of what the string value will be.

Later in assertResourceResponse() it appears the test (incorrectly) was written to depend upon GetDocumentFromResponseTrait::getDocumentFromResponse() returning NULL on invalid JSON which is off-spec for the API.

In Oembed:
core/modules/media/src/OEmbed/ProviderRepository.php:116 appears to be checked by looking at the value being empty rather than looking for an error (another off-spec use of the API).
core/modules/media/src/OEmbed/ResourceFetcher.php:67 appears to check the internal use of json_decode with json_last_error(). #3186415: Make oEmbed resource fetcher more tolerant of unexpected Content-Type headers → called out that the JSON Decoder failed to throw an exception.

Essentially: Legitimate test failures caused by off-api usage of methods.

We have a passing test stack now. Setting needs review to get eyes on below:

$ grep -R Json::decode *|grep -v Test

The following entries are already converted to handle the exception (based on failing tests):

core/modules/system/src/SecurityAdvisories/SecurityAdvisoriesFetcher.php:        $json_payload = Json::decode($response);
core/modules/jsonapi/tests/src/Traits/GetDocumentFromResponseTrait.php:      $document = Json::decode((string) $response->getBody());
core/modules/media/src/OEmbed/ResourceFetcher.php:        $data = Json::decode($content);
core/modules/media/src/OEmbed/ProviderRepository.php:      $providers = Json::decode((string) $response->getBody());

We may want to convert the following areas of code with no apparent failing tests to catch the exception. If so do we need to do that here or is that a followup? If we need to do that here can we skip the test gate? These are well out of scope of the bug being reported here.

core/modules/announcements_feed/src/AnnounceFetcher.php:        $announcements = Json::decode($feed_content);	
core/modules/jsonapi/src/Controller/EntityResource.php:      $document = Json::decode($request->getContent());
core/modules/jsonapi/src/Controller/EntityResource.php:    $body = Json::decode($request->getContent());
core/modules/ckeditor5/src/Plugin/Editor/CKEditor5.php:    $toolbar_items = Json::decode($json);

Unsure on if we want these to be patched, or if we want the exceptions to rise through the stack:

core/modules/package_manager/src/PathExcluder/UnknownPathExcluder.php:    $repositories = Json::decode($repositories);
core/modules/package_manager/src/PathExcluder/UnknownPathExcluder.php:    $extra = Json::decode($this->composerInspector->getConfig('extra', $packages['drupal/core']->path . '/composer.json'));
core/modules/package_manager/src/Validator/AllowedScaffoldPackagesValidator.php:    $extra = Json::decode($this->composerInspector->getConfig('extra', $path . '/composer.json'));
core/modules/package_manager/src/Validator/ComposerPatchesValidator.php:      $extra = Json::decode($this->composerInspector->getConfig('extra', $working_dir));
core/modules/package_manager/src/Validator/PhpTufValidator.php:    $repositories = Json::decode($repositories);
core/modules/contextual/contextual.module:    $element['#links'] = Json::decode(rawurldecode($encoded_links));

package_manager: We might want those errors to show, in either case as the module is experimental can we deffer that to package_manager project to figure out before it makes it to stable?

contextual.module: This appears to be developer facing, we may want this to rise through the stack.

Suggest we do not consider patching:
core/modules/media/src/OEmbed/ResourceFetcher.php: return Json::decode($data);
This is decoding data that just went through encode(), if this fails something is significantly wrong with the deployment.

Needs a manual rebase. This is one may need to ping a framework manager or core committer (usually same group)

Manual rebase (conflict was due to new comments in ResourceTestBase.php).

Reran jobs with random failures.