Preparing for
#1920902: Add a Drupal Yaml wrapper so we can default to PECL Yaml component if it is available
#2190723: Add a KeyValueStore\FileStorage to replace e.g. ConfigStorage

Problem

  1. Drupal\Component\Utility\Json produces JSON that is specific to Drupal's requirements.
  2. There is no point to provide this class in Component, because json_encode() + json_decode() are native PHP functions and this utility class just passes some hard-coded parameters (again, for consistency within Drupal).
  3. Json is actually a serialization format we want to standardize on.

Proposed solution

  1. Move Drupal\Component\Utility\Json into Drupal\Component\Serialization\Json.

Ultimate goal

Drupal\Component\Serialization\Json
Drupal\Component\Serialization\PhpSerialize
Drupal\Component\Serialization\Yaml
Drupal\Component\Serialization\Xml
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Status: Active » Needs review
FileSize
10.16 KB

Status: Needs review » Needs work

The last submitted patch, 1: json.1.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

1: json.1.patch queued for re-testing.

sun’s picture

1: json.1.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: json.1.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

1: json.1.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: json.1.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

1: json.1.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: json.1.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

1: json.1.patch queued for re-testing.

dawehner’s picture

I just don't see why this particular code is specific to drupal. Other projects (like drupal people writing php only projects)
could profit. Drupal\Component\Serialization could also work here.

Status: Needs review » Needs work

The last submitted patch, 1: json.1.patch, failed testing.

sun’s picture

I just don't see why this particular code is specific to drupal.

The only code that exists in this class is this:

    return json_encode($variable, JSON_HEX_TAG | JSON_HEX_APOS | JSON_HEX_AMP | JSON_HEX_QUOT);

All of these flags are native PHP constants:

http://www.php.net/manual/en/function.json-encode.php
http://www.php.net/manual/en/json.constants.php

This code is specific to Drupal, because the one and only purpose is to harmonize and standardize JSON-encoded strings throughout Drupal modules.

If I'd need to encode variables into JSON in my own non-Drupal component/project/code, then I assure you that I won't use a utility helper class from Drupal to do that, because that would be a totally senseless dependency — json_encode() is readily available.

Other projects (like drupal people writing php only projects) could profit.

Profit from what?

sun’s picture

Status: Needs work » Needs review

1: json.1.patch queued for re-testing.

sun’s picture

Hm. Perhaps we're talking past each other?

Are you saying that you'd be fine if the class was moved into Drupal\Component\Serialization instead of Drupal\Core?

The reason for why I'm suggesting Drupal\Core\Serialization is that all of the serialization classes are hard-coding various flags that are, in essence, just simply no more and no less than Drupal's preferences; i.e.,

  1. This Json class hard-codes some json_encode() flags, because we expect most JSON to get rendered on an HTML page.
  2. The Yaml class hard-codes flags for Yaml\Dumper to comply with Drupal coding standards; i.e., indent 2 spaces instead of 4, and don't switch to inline YAML after a certain nesting level.
  3. Only the PhpSerialize class hard-codes nothing, because PHP serialize() does not support any flags.

I assume the point where we digress is that

  1. You're saying that the concept of a standardized serialization class toolset could be useful for others.
  2. I'm saying that the concept might be, but the actual implementations are not, because the implementations are hard-coding assumptions that are special to Drupal.

In order to make the concept a truly useful standalone component, we'd have to abstract the hard-coded assumptions, bloat the whole shebang with a serialization manager/configurator service and/or other ways to e.g. pass or statically set the configuration... The end result of such an effort would be something along the lines of Symfony's Serialization component, which is a super-complex beast of code.

However, if it's really just the Component vs. Core aspect, and if my arguments are not able to convince you, then I'm happy to concede and simply move the serialization classes into Drupal\Component\Serialization — I personally think that it does not make sense, but I really want/need to move forward on this.

dawehner’s picture

Thank you for your long answer and patience. It is convincing that the actual implementation is tight to the requirements of drupal, so if other projects
would like something similar they should set their own rules. We should be pragmatic here.

My point was more on an abstract level. We should try to allow other people to reuse our code and not remove that possiblity again.

+++ b/core/modules/views/tests/Drupal/views/Tests/Controller/SystemControllerTest.php
index c1a8343..9acea78 100644
--- a/core/tests/Drupal/Tests/Component/Utility/JsonTest.php

--- a/core/tests/Drupal/Tests/Component/Utility/JsonTest.php
+++ b/core/tests/Drupal/Tests/Component/Utility/JsonTest.php

Ensure to move the tests out of the component directory

sun’s picture

FileSize
10.36 KB
670 bytes

Moved PHPUnit test accordingly.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: json.18.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
10.4 KB

Pure git reroll.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: 2208609-21.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

21: 2208609-21.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 21: 2208609-21.patch, failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
23.61 KB
13.21 KB

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 26: json.26.patch, failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
23.18 KB

Hah. HEAD is moving fast today. ;)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

json.28.patch no longer applies.

error: core/modules/block/lib/Drupal/block/BlockListController.php: No such file or directory

sun’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
23.15 KB

The last submitted patch, 28: json.28.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 30: json.30.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

30: json.30.patch queued for re-testing.

sun’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

json.30.patch no longer applies.

error: patch failed: core/modules/locale/locale.module:10
error: core/modules/locale/locale.module: patch does not apply

sun’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
23.17 KB

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 36: json.36.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

36: json.36.patch queued for re-testing.

sun’s picture

Status: Needs review » Reviewed & tested by the community
damiankloip’s picture

+1, still looks good.

sun’s picture

Patch still applies cleanly (despite the high core commit volume these days).

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 36: json.36.patch, failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
23.22 KB

Merged 8.x.

damiankloip’s picture

Looks good.

sun’s picture

FileSize
23.55 KB
23.6 KB

After further discussion on #2208633: Add a Yaml class to Drupal\Component\Serialization + IRC...:

Moved Drupal\Core\Serialization into Drupal\Component\Serialization.

sun’s picture

Issue summary: View changes

Updating issue summary accordingly.

sun’s picture

Title: Move Json utility class into Drupal\Core\Serialization » Move Json utility class into Drupal\Component\Serialization

Haha + issue title :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 45: json.45.patch, failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
23.53 KB

diff context conflict

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 49: json.49.patch, failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
23.53 KB

diff context conflict

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 51: json.51.patch, failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
23.55 KB

Merged 8.x

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 53: json.53.patch, failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
23.83 KB
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

json.55.patch no longer applies.

error: patch failed: core/lib/Drupal/Core/Asset/JsCollectionRenderer.php:6
error: core/lib/Drupal/Core/Asset/JsCollectionRenderer.php: patch does not apply

Jalandhar’s picture

Status: Needs work » Needs review
FileSize
23.83 KB

Rerolled patch.

sun’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

Manually merged, verified, and confirmed the re-roll.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

  • Commit 2681750 on 8.x by webchick:
    Issue #2208609 by sun, Jalandhar, damiankloip: Move Json utility class...
hass’s picture

https://drupal.org/node/2219113 is very confusing now and only ~3 weeks old.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.