Json and PHP serializers should throw exception on failure

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.

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

Reran jobs with random failures.

I mentioned in Slack that the raised concern with Database Cache Backend likely needs significant architectural decisions that should not be rushed into this issue however I would implement it if it is the only way to get this to move forward.

It turns out that there already is an issue to handle the request in #2872694: Database backends cannot deal with corrupt serialized data: app completey broken.

Setting back to NR.

Responded to MR comment.

wondering whether this needs to be a deprecation (or unsilenced E_USER_WARNING) changing to an exception in 12.x

My personal opinion is no. I can understand why this would be asked since this can be disruptive and given by the code being corrected there is clear proof that some implementations have misused this

However the API is well defined, the proper response is an exception. Code that has properly implemented SerilizationInterface::decode would already have a try/catch block surrounding it.

Today 3rd parties could replace the decoder using a class alias at the auto loader meaning the code that is broken by us fixing this can already be broken by a contrib module following reasonable design standards.

The FALSE return was/is an internal component of the JSON decode method that leaked and should not have been utilized.

As noted in the issue summary, this failure to throw an exception as documented on the API can lead to undetected data loss for those applications that properly follow the documented API.

This bug fundamentally breaks the ability to use DI of the serialization services.

I have no proven case to show this however a worst case fear is that silent data corruption could lead to security incidents.

CR created https://www.drupal.org/node/3519175

it needs to use set_error_handler() to catch warnings too (phpserialize)

The code in this MR very intentionally checks the FALSE return as documented in the PHP Manual

false is returned both in the case of an error and if unserializing the serialized false value. It is possible to catch this special case by comparing data with serialize(false) or by catching the issued E_WARNING. 

Technically can't classes being unserialized trigger an E_WARNING through the stack without impacting the ability to unserialize the string as a whole? Is there a way to positively assert that that E_WARNING came from just the unserialize() method and not another method (or that the error handler might be replaced inside the __wakeup()/__unserialize() methods? )