Skip to content

Commit

Permalink
Merge pull request #1169 from Glavic/phpcs-fix
Browse files Browse the repository at this point in the history 
Fix for phpcs to pass
  • Loading branch information
@Glavic
Glavic authored Mar 9, 2018
2 parents 3584051 + 08bf9b9 commit a05bed2
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 8 deletions.
4 changes: 2 additions & 2 deletions .travis.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ install:
fi
script:
- vendor/bin/phpunit --verbose --coverage-clover=coverage.xml
- if [[ $phpstan = 'true' ]]; then vendor/bin/phpstan analyse --configuration phpstan.neon --level 3 src tests; fi
- composer phpunit
- if [[ $phpstan = 'true' ]]; then composer phpstan; fi

after_success:
- if [[ $coverage = 'true' ]]; then bash <(curl -s https://codecov.io/bash); fi
Expand Down
5 changes: 3 additions & 2 deletions composer.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@
"@phpunit",
"@phpcs"
],
"phpunit": "phpunit",
"phpcs": "php-cs-fixer fix -v --diff --dry-run"
"phpunit": "phpunit --verbose --coverage-clover=coverage.xml",
"phpcs": "php-cs-fixer fix -v --diff --dry-run",
"phpstan": "phpstan analyse --configuration phpstan.neon --level 3 src tests"
}
}
9 changes: 5 additions & 4 deletions src/Carbon/Carbon.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,8 @@ public function __construct($time = null, $tz = null)
$timezone = static::safeCreateDateTimeZone($tz);
// @codeCoverageIgnoreStart
if ($isNow && !isset($testInstance) && (
version_compare(PHP_VERSION, '7.1.0-dev', '<')) ||
version_compare(PHP_VERSION, '7.1.0-dev', '<')
) ||
version_compare(PHP_VERSION, '7.1.3-dev', '>=') && version_compare(PHP_VERSION, '7.1.4-dev', '<')
) {
$dateTime = new DateTime('now', $timezone);
Expand Down Expand Up @@ -3846,12 +3847,12 @@ public static function fromSerialized($value)
/**
* The __set_state handler.
*
* @param array $array
* @param array $state
*
* @return static
*/
public static function __set_state($array)
public static function __set_state(array $state)
{
return static::instance(parent::__set_state($array));
return static::instance(parent::__set_state($state));
}
}

6 comments on commit a05bed2

@acasar
Copy link
Contributor

@acasar acasar commented on a05bed2 Mar 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This array typehint is a breaking change.

Laravel is overriding __set_state

@RyanThompson
Copy link

Choose a reason for hiding this comment

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

laravel/framework#23459

@kylekatarnls
Copy link
Collaborator

Choose a reason for hiding this comment

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

Laravel can just remove this method and increase the minimal Carbon dependency version.

@kylekatarnls
Copy link
Collaborator

Choose a reason for hiding this comment

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

But we will revert and patch as it may be better to match the DateTime method.

@Glavic
Copy link
Collaborator Author

@Glavic Glavic commented on a05bed2 Mar 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

http://php.net/manual/en/datetime.set-state.php

public static DateTime DateTime::__set_state ( array $array )

@kylekatarnls
Copy link
Collaborator

Choose a reason for hiding this comment

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

All array typing in the documentation are not always strict. And as extending __set_state without the hint did not throw exception it should mean this method has not strict typing control.

So now than libraries (not only Laravel) extend Carbon and maybe override this method, it would be safer to not keeping the array since we don't really need it.

Please sign in to comment.