Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Drupal\Component\Utility\Json
produces JSON that is specific to Drupal's requirements.- There is no point to provide this class in
Component
, becausejson_encode()
+json_decode()
are native PHP functions and this utility class just passes some hard-coded parameters (again, for consistency within Drupal). Json
is actually a serialization format we want to standardize on.
Proposed solution
- Move
Drupal\Component\Utility\Json
intoDrupal\Component\Serialization\Json
.
Ultimate goal
Drupal\Component\Serialization\Json
Drupal\Component\Serialization\PhpSerialize
Drupal\Component\Serialization\Yaml
Drupal\Component\Serialization\Xml
Comment | File | Size | Author |
---|---|---|---|
#57 | json.57.patch | 23.83 KB | Jalandhar |
#55 | json.55.patch | 23.83 KB | sun |
#53 | json.53.patch | 23.55 KB | sun |
#51 | json.51.patch | 23.53 KB | sun |
#49 | json.49.patch | 23.53 KB | sun |
Comments
Comment #1
sunComment #3
sun1: json.1.patch queued for re-testing.
Comment #4
sunComment #5
sun1: json.1.patch queued for re-testing.
Comment #7
sun1: json.1.patch queued for re-testing.
Comment #9
sun1: json.1.patch queued for re-testing.
Comment #11
sun1: json.1.patch queued for re-testing.
Comment #12
dawehnerI 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.
Comment #14
sunThe only code that exists in this class is this:
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.Profit from what?
Comment #15
sun1: json.1.patch queued for re-testing.
Comment #16
sunHm. 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 ofDrupal\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.,json_encode()
flags, because we expect most JSON to get rendered on an HTML page.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.serialize()
does not support any flags.I assume the point where we digress is that
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 intoDrupal\Component\Serialization
— I personally think that it does not make sense, but I really want/need to move forward on this.Comment #17
dawehnerThank 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.
Ensure to move the tests out of the component directory
Comment #18
sunMoved PHPUnit test accordingly.
Comment #19
dawehnerThank you
Comment #21
damiankloip CreditAttribution: damiankloip commentedPure git reroll.
Comment #22
sunComment #24
tim.plunkett21: 2208609-21.patch queued for re-testing.
Comment #26
sunUpdated all new instances after #2093173: Remove all calls to drupal_json_decode(), use \Drupal\Component\Utility\Json::decode()
Comment #28
sunHah. HEAD is moving fast today. ;)
Comment #29
alexpottjson.28.patch no longer applies.
Comment #30
sunComment #33
sun30: json.30.patch queued for re-testing.
Comment #34
sunComment #35
alexpottjson.30.patch no longer applies.
Comment #36
sunComment #38
sun36: json.36.patch queued for re-testing.
Comment #39
sunComment #40
damiankloip CreditAttribution: damiankloip commented+1, still looks good.
Comment #41
sunPatch still applies cleanly (despite the high core commit volume these days).
Comment #43
sunMerged 8.x.
Comment #44
damiankloip CreditAttribution: damiankloip commentedLooks good.
Comment #45
sunAfter further discussion on #2208633: Add a Yaml class to Drupal\Component\Serialization + IRC...:
Moved
Drupal\Core\Serialization
intoDrupal\Component\Serialization
.Comment #46
sunUpdating issue summary accordingly.
Comment #47
sunHaha + issue title :)
Comment #49
sundiff context conflict
Comment #51
sundiff context conflict
Comment #53
sunMerged 8.x
Comment #55
sunComment #56
alexpottjson.55.patch no longer applies.
Comment #57
Jalandhar CreditAttribution: Jalandhar commentedRerolled patch.
Comment #58
sunManually merged, verified, and confirmed the re-roll.
Comment #59
webchickCommitted and pushed to 8.x. Thanks!
Comment #61
hass CreditAttribution: hass commentedhttps://drupal.org/node/2219113 is very confusing now and only ~3 weeks old.