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

Fix attributes type #66

Merged
merged 3 commits into from
Jan 15, 2017
Merged

Fix attributes type #66

merged 3 commits into from
Jan 15, 2017

Conversation

mhor
Copy link
Owner

@mhor mhor commented Jan 13, 2017

related (#65)

  • cast to int/float
  • add unit tests

@mhor mhor assigned mhor and unassigned mhor Jan 14, 2017
@mhor
Copy link
Owner Author

mhor commented Jan 14, 2017

  @greg0ire could you review my pull request. If it's ok for you I will merge the PR, and release a new
version.

@@ -14,11 +14,11 @@ class Duration implements AttributeInterface
private $milliseconds;

/**
* @param $duration
* @param string $duration
Copy link
Contributor

@greg0ire greg0ire Jan 14, 2017

Choose a reason for hiding this comment

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

string|int, because the class should not know what it will be called with. In php7, you will use int as type hinting (which accepts numeric strings IIRC)

Copy link
Contributor

@greg0ire greg0ire Jan 14, 2017

Choose a reason for hiding this comment

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

By default, PHP will coerce values of the wrong type into the expected scalar type if possible. For example, a function that is given an integer for a parameter that expects a string will get a variable of type string.

So when you drop php 5 (on next major release?), you can safely add int as type hinting indeed.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Fixed!

@mhor mhor merged commit 53961b3 into master Jan 15, 2017
@mhor mhor deleted the fix-attributes-type branch January 15, 2017 21:31
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.

2 participants