DDC-3863 add a `json` type that doesn't have the flaws of `json_array` #892

Taluu · 2015-08-04T11:36:10Z

As discussed in #891, the JsonArrayType was deprecated in favor of a new JsonType. :}

doctrinebot · 2015-08-04T11:36:22Z

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DBAL-1277

We use Jira to track the state of pull requests and the versions they got
included in.

stof · 2015-08-05T14:41:25Z

lib/Doctrine/DBAL/Types/JsonArrayType.php

}
+

the extra empty line should be removed

deeky666 · 2015-08-21T17:33:40Z

lib/Doctrine/DBAL/Types/JsonType.php

+            return null;
+        }
+
+        return json_encode($value);


Shouldn't we throw an exception here, too if encoding fails?

Should throw some variant of ConversionException with the return value of json_last_error().

Should I do it too for the convertToPHPValue ?

No - the logic there is fine - ConversionException is missing a static constructor for this PHP->database scenario - there should probably be a ConversionException::serializationFailed method or something like that.

@Taluu the PR seems fine so far but still throwing a ConversionException if json_encode() fails is missing.

Yep, I was on it, and I just updated it (and rebased it with the master branch)

stof · 2015-09-24T17:25:03Z

lib/Doctrine/DBAL/Types/JsonType.php

+    {
+        if (null === $value) {
+            return null;
+        }


should this actually be the case ? shouldn't it be converted to a json null instead ?

Not quite sure if it should be encoded too, if the field can be nullable ? Otherwise, it gives a "null" string when encoded in json...

@stof @Taluu Kind of debatable -

php > var_dump(json_decode('null') === json_decode(null)); bool(true)

If the platform has native json support (like postgresql) it will be stored as null. If the platform doesn't have native json support, then storing it as a SQL null will achieve much the same result, as deserializing the value to PHP value will be the same whether it's 'null' or NULL

Maybe, but on json_encode, it differs ;

Psy Shell v0.6.1 (PHP 7.0.2 — cli) by Justin Hileman >>> json_encode('null'); => ""null"" >>> json_encode(null); => "null"

And then on a json_decode call :

Psy Shell v0.6.1 (PHP 7.0.2 — cli) by Justin Hileman >>> json_decode(null); => null >>> json_decode('null'); => null >>> json_decode('"null"'); => "null"

So I think returning a null value when the value is actually null should be more appropriate (Sorry for the late answer...)

I tend to prefer SQL NULL whenever possible as it seems more convenient than having a JSON null string stored (might also be more efficient storage wise...). Also it avoids unnecessary json_decode() calls for converting back to PHP which might improve performance. If nobody disagrees, I'll accept the current implementation.

What is also coming to my mind is database queries against JSON columns where you want to filter "empty" (NULL) values which would not be possible via IS NULL if we would put JSON NULL into DB. So returning null here is the better approach IMO.

Taluu · 2016-01-25T12:49:34Z

The PR was updated with the latest comment and remarks. cc @deeky666 @zeroedin-bill @stof (sorry for the delay)

deeky666 · 2016-01-25T13:19:41Z

lib/Doctrine/DBAL/Types/JsonType.php

+    // extracted from symfony's php 5.5 polyfill
+    // @link https://github.com/symfony/polyfill-php55/blob/master/Php55.php
+    // @link http://nl1.php.net/manual/en/function.json-last-error-msg.php
+    private static function json_last_error_msg()


Why static? This method is not used in static context so we should make it non-static. Also please don't use underscore naming. I'd propose something like getLastErrorMessage().

changed. I kind of copied the polyfill, and static seemed ok to be used here as it does not depend on any context. But I removed the static, as it doesn't really matter

deeky666 · 2016-01-25T13:27:31Z

lib/Doctrine/DBAL/Types/ConversionException.php

@@ -99,4 +99,16 @@ static public function conversionFailedInvalidType($value, $toType, array $possi
            implode(', ', $possibleTypes)
        ));
    }
+
+    static public function conversionFailedSerialization($value, $format, $error, \Exception $previous = null)


Not sure if we really need to introduce another method here. Maybe it would suffice the purpose by sticking to conversionFailedFormat()

/cc @Ocramius @zeroedin-bill

That is also what I thought but as @zeroedin-bill suggested it, I went with it

@Taluu yeah sorry then for the extra effort. Could you be so kind to just use conversionFailedFormat() then? Also you might just put the error message into $expectedFormat. Like:

<?php // snippet if (JSON_ERROR_NONE !== json_last_error()) { throw ConversionException::conversionFailedFormat( $value, Type::JSON, 'json (' . $this->getLastErrorMessage() . ')' ); }

@Taluu ah no wait.

The exception message says Could not convert database value to Doctrine type which would be wrong then.

I'm leaving this as is then, unless something better can be found ?

I'm not quite sure the message format is good too though

Taluu · 2016-02-29T10:19:41Z

Ping. Any news around here ?

Thanks

Taluu · 2016-06-08T12:24:01Z

Sorry to stir it up again, but any news around here ? /cc @deeky666 @zeroedin-bill @Ocramius

deeky666 · 2016-06-08T14:40:49Z

LGTM. /cc @zeroedin-bill @Ocramius merge?

Ocramius · 2016-06-15T15:20:43Z

LGTM. @zeroedin-bill, can you check it and press the big green scary button? :-)

billschaller · 2016-06-15T15:21:50Z

Driving all day, sry
On Jun 15, 2016 11:20 AM, "Marco Pivetta" notifications@github.com wrote:

Assigned #892 #892 to
@zeroedin-bill https://github.com/zeroedin-bill.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#892 (comment), or mute the
thread
https://github.com/notifications/unsubscribe/AEEbYQhpqLjs-55SirJX1yxK0RGK4ykOks5qMBhUgaJpZM4FlPPC
.

Ocramius · 2016-06-15T15:25:54Z

Assigned it to you - whenever you got time :-)

Doesn't need merging back into 2.5, so it's as quick as the UI interaction,
after review.

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

On 15 June 2016 at 17:21, Bill Schaller notifications@github.com wrote:

Driving all day, sry
On Jun 15, 2016 11:20 AM, "Marco Pivetta" notifications@github.com
wrote:

Assigned #892 #892 to
@zeroedin-bill https://github.com/zeroedin-bill.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#892 (comment), or mute the
thread
<
https://github.com/notifications/unsubscribe/AEEbYQhpqLjs-55SirJX1yxK0RGK4ykOks5qMBhUgaJpZM4FlPPC

.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#892 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAJakIvj1Jhu4FA1YEIthqBmUf-EHKr6ks5qMBiPgaJpZM4FlPPC
.

guilhermeblanco · 2016-06-15T15:29:21Z

"Even though I walk through the valley of the shadow of death, I fear no evil"

Merged... =)

Taluu force-pushed the json-unflawed branch from ae5983b to 0163f63 Compare August 4, 2015 11:41

stof reviewed Aug 5, 2015
View reviewed changes

Taluu force-pushed the json-unflawed branch 4 times, most recently from 49945bb to 54f73b5 Compare August 5, 2015 15:06

deeky666 reviewed Aug 21, 2015
View reviewed changes

deeky666 added Requires Review Improvement labels Aug 21, 2015

stof reviewed Sep 24, 2015
View reviewed changes

doctrinebot mentioned this pull request Dec 6, 2015

DBAL-1277: [GH-892] Json unflawed Type (DDC-3863) #1230

Closed

deeky666 added Types In Review Requires Feedback Type Conversion and removed Requires Review labels Jan 8, 2016

Taluu added 2 commits January 25, 2016 13:19

Add a proper and unflawed JsonType

7604e36

Deprecate old JsonArrayType

dee0141

Taluu force-pushed the json-unflawed branch from 54f73b5 to e26e0a8 Compare January 25, 2016 12:48

Taluu force-pushed the json-unflawed branch from e26e0a8 to ea03e1a Compare January 25, 2016 12:53

deeky666 reviewed Jan 25, 2016
View reviewed changes

Taluu force-pushed the json-unflawed branch from ea03e1a to 8b7f768 Compare January 25, 2016 13:22

Throws an ConversionException if the json encoding fails

33f5596

Taluu force-pushed the json-unflawed branch from 8b7f768 to 33f5596 Compare January 25, 2016 13:25

deeky666 reviewed Jan 25, 2016
View reviewed changes

deeky666 added this to the 2.6 milestone Jun 8, 2016

deeky666 removed the Requires Feedback label Jun 8, 2016

Ocramius assigned billschaller Jun 15, 2016

guilhermeblanco self-assigned this Jun 15, 2016

guilhermeblanco merged commit 7d01ce7 into doctrine:master Jun 15, 2016

Taluu deleted the json-unflawed branch June 15, 2016 15:31

deeky666 removed the In Review label Jun 16, 2016

Ocramius changed the title ~~Json unflawed Type (DDC-3863)~~ DDC-3863 add a json type that doesn't have the flaws of json_array Jul 22, 2017

GwendolenLynch mentioned this pull request Aug 31, 2017

[3.3] Support DBAL 2.6 bolt/bolt#6940

Merged

glen-84 mentioned this pull request Jan 19, 2022

DDC-3863: Wrong return if value is null in JsonArrayType doctrine/orm#4715

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DDC-3863 add a `json` type that doesn't have the flaws of `json_array` #892

DDC-3863 add a `json` type that doesn't have the flaws of `json_array` #892

Taluu commented Aug 4, 2015

doctrinebot commented Aug 4, 2015

stof Aug 5, 2015

Taluu Aug 5, 2015

deeky666 Aug 21, 2015

billschaller Sep 11, 2015

Taluu Sep 24, 2015

billschaller Sep 25, 2015

deeky666 Jan 25, 2016

Taluu Jan 25, 2016

stof Sep 24, 2015

Taluu Sep 24, 2015

billschaller Sep 25, 2015

Taluu Jan 25, 2016

deeky666 Jan 25, 2016

deeky666 Jun 8, 2016

Taluu commented Jan 25, 2016

deeky666 Jan 25, 2016

Taluu Jan 25, 2016

deeky666 Jan 25, 2016

deeky666 Jan 25, 2016

Taluu Jan 25, 2016

deeky666 Jan 25, 2016

deeky666 Jan 25, 2016

deeky666 Jan 25, 2016

Taluu Jan 25, 2016

Taluu Jan 25, 2016

Taluu commented Feb 29, 2016

Taluu commented Jun 8, 2016

deeky666 commented Jun 8, 2016

Ocramius commented Jun 15, 2016

billschaller commented Jun 15, 2016

Ocramius commented Jun 15, 2016

guilhermeblanco commented Jun 15, 2016 •

edited

Loading

DDC-3863 add a json type that doesn't have the flaws of json_array #892

DDC-3863 add a json type that doesn't have the flaws of json_array #892

Conversation

Taluu commented Aug 4, 2015

doctrinebot commented Aug 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Taluu commented Jan 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Taluu commented Feb 29, 2016

Taluu commented Jun 8, 2016

deeky666 commented Jun 8, 2016

Ocramius commented Jun 15, 2016

billschaller commented Jun 15, 2016

Ocramius commented Jun 15, 2016

guilhermeblanco commented Jun 15, 2016 • edited Loading

DDC-3863 add a `json` type that doesn't have the flaws of `json_array` #892

DDC-3863 add a `json` type that doesn't have the flaws of `json_array` #892

guilhermeblanco commented Jun 15, 2016 •

edited

Loading