Json_Array type returns empty array when used with nullable annotation #968

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
10 participants
Contributor

localheinz commented Mar 4, 2014

It says in http://www.doctrine-project.org/jira/browse/DBAL-446 that this issue has been fixed, but apparently, it hasn't.

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/DDC-3009

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

Owner

Ocramius commented Mar 4, 2014

@localheinz probably due to a regression, since json_array was introduced.

Can you check what tests fail when removing the conditional at https://github.com/doctrine/dbal/blob/149f18001a459c22e1f2a87903e455eb89c1b4de/lib/Doctrine/DBAL/Types/JsonArrayType.php#L57-L59 ?

Owner

Ocramius commented Mar 4, 2014

Yeah, sounds weird...
Since json natively understands null, should json_array fields from the ORM always be not-nullable?

Contributor

localheinz commented Mar 4, 2014

Not sure. What would you say?

Owner

Ocramius commented Mar 4, 2014

Well, I'd convert null to 'null' here, to be honest. At application level, there's no real difference between an SQL NULL and a JSON 'NULL', and that's why this test/feature was set in place.

@deeky666 opinions?

Contributor

localheinz commented Mar 4, 2014

@Ocramius

Let's see what Travis says.

@stof stof commented on the diff Mar 4, 2014

composer.json
@@ -16,7 +16,7 @@
"php": ">=5.3.2",
"ext-pdo": "*",
"doctrine/collections": "~1.1",
- "doctrine/dbal": ">=2.5-dev,<2.6-dev",
+ "doctrine/dbal": "dev-bugfix/nullable-json-array-mapped-to-empty-array",
@stof

stof Mar 4, 2014

Member

this should not be in the PR

@localheinz

localheinz Mar 4, 2014

Contributor

@stof

Sure, needs to be changed if doctrine/dbal#538 gets merged.

@stof stof commented on an outdated diff Mar 4, 2014

...s/Doctrine/Tests/ORM/Functional/Ticket/DDC446Test.php
@@ -0,0 +1,72 @@
+<?php
+
+namespace Doctrine\Tests\ORM\Functional\Ticket;
+
+require_once __DIR__ . '/../../../TestInit.php';
@stof

stof Mar 4, 2014

Member

This should be removed as it is already the PHPUnit bootstrap file (yeah, I know we have lots of places using it because it was not cleaned when cleaning the PHPUnit setup)

Contributor

localheinz commented Mar 4, 2014

@stof

Any idea how to reference the feature branch in my fork of doctrine/dbal?

Member

deeky666 commented Mar 4, 2014

@Ocramius Not sure why it doesn't return null instead of array(). @schmittjoh introduced the type in DBAL. Maybe ask him if it was done by intention. Also I'm not sure if we can change this in DBAL without marking it as BC break.

Member

schmittjoh commented Mar 4, 2014

I had preferred to always rely on this being an array and not having to check for null, hence the name json_array, not something like json_value. I'd say better introduce a second field with a flag if you need to distinguish between null vs. empty array or even a json_value type.

At this point, such a change would be a BC break which at least from my POV is not worth it.

till commented Mar 4, 2014

@localheinz

Add a repository:

{
  "repositories": {
    {"type": "vcs", "url": "https://github.com/localheinz/dbal.git"}
  }
}

Then require the branch.

Contributor

localheinz commented Mar 4, 2014

@till

It's already in there, but without .git.

Member

deeky666 commented Mar 4, 2014

To be honest I agree with @schmittjoh. A nullable field mapping in ORM defines that it is nullable at database level and stored as such but IMO has nothing to do with the value converted back from database to PHP.
@localheinz Can you explain what exact issue you have with getting an empty array vs. null? Otherwise I think we can't accept a BC break here. If you come up with a real use case for this we might consider adding an optional flag to return null instead of empty array but I don't think this is really useful.

Contributor

localheinz commented Mar 4, 2014

@deeky666 @schmittjoh

Maybe it's an issue of understanding the nullable option, then.

I understood that this applies to being able to have null values for a column, but I expected the same stuff to get out that was put in, i.e. null. In a project I have an integration test similar to DDC446Test that fails.

Searching, I found the issue and thought I'd submit as a PR.

We've already fixed our tests by changing the assertions.

pkracht commented Mar 4, 2014

@schmittjoh if you dont want to check for null, just make the field not nullable and set the default value to "[]" instead of NULL.

@deeky666 i dont agree with that it has nothing to do with the value converted back from database, because of you are loosing information: NULL => array() and "[]" => array() - how would you get the difference after conversion?

Member

deeky666 commented Mar 8, 2014

@pkracht You are right. Sorry I haven't thougtht about it in that way. I can understand the need for this fix but there is still the question of BC for people relying on the current behaviour :(
The reason I am a bit sceptical is this, which is similar to this issue IMO and there is a good reason for this because changes in the type conversion always affect a lot of things. All that I am saying is that we should not rush things when it comes to changes in DBAL types. This is rather critical IMHO.

@ghost
Contributor

ghost commented Apr 1, 2014

@pkracht
Adding a json_array field with a default value of [] results in an exception when you attempt to update the schema

[Doctrine\DBAL\DBALException]
  An exception occurred while executing 'ALTER TABLE event ADD completed_steps LONGTEXT DEFAULT '[]' NOT NULL COMMENT '(DC2Type:json_array)'':  

SQLSTATE[42000]: Syntax error or access violation: 1101 BLOB/TEXT column 'completed_steps' can't have a default value
Member

deeky666 commented Apr 1, 2014

@v3labs MySQL does not support default values on LOB type columns. As MySQL also has no native JSON type, this is a limitation for that vendor then.

@ghost
Contributor

ghost commented Apr 1, 2014

I know that. It was just an example why the proposed solution is not feasible.

Owner

guilhermeblanco commented Apr 17, 2014

This is a DBAL specific issue which IMHO should be addressed on 3.0 only.
Closing related ORM issue as this test is a "duplicate" of DBAL one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment