Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable strict mode, fix type juggling and drop some runtime coercion broken by design #916

Merged
merged 1 commit into from Apr 26, 2018

Conversation

Majkl578
Copy link
Contributor

Q A
Bug fix? no
New feature? no
BC breaks? yes

This breaks some type juggling which is incompatible with strict mode by design (like deserializing object with __toString()).

@@ -108,22 +110,22 @@ public function accept($data, array $type = null, Context $context)
return $visitor->visitNull($data, $type);

case 'string':
return $visitor->visitString($data, $type);
return $visitor->visitString((string)$data, $type);
Copy link
Contributor Author

@Majkl578 Majkl578 Apr 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these changes are potentially BC breaking, but not by this PR, by already existing type hints. This only eliminates runtime type juggling.

$this->assertTrue($context->shouldSerializeNull());

$context->setSerializeNull("0");
$this->assertFalse($context->shouldSerializeNull());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is BC break-ish, incompatible with strict typing by design.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree with getting rid of it

@@ -203,34 +205,18 @@ public function getPrimitiveTypes()
'type' => 'boolean',
'data' => true,
),
array(
Copy link
Contributor Author

@Majkl578 Majkl578 Apr 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these are removed, type coerction of this sort is incompatible with strict types by design.

@@ -385,15 +387,6 @@ public function testDeserializingNull()
$this->markTestSkipped('Not supported in XML.');
}

public function testDeserializeWithObjectWithToStringMethod()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Objects with __toString() are not string, therefore removed as incompatible with strict types.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, this "feature" can be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has simple user-land workaround: (string) $stringishObject, but imho that doesn't make much sense anyway. 😀

@goetas goetas added this to the v2.0 milestone Apr 26, 2018
@goetas
Copy link
Collaborator

goetas commented Apr 26, 2018

Checked with #917, works!

@goetas goetas merged commit 51e62ed into schmittjoh:master Apr 26, 2018
@Majkl578 Majkl578 deleted the strict-types branch April 26, 2018 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants