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

[WIP] Upgrade PHP 7.0 #511

Merged
merged 40 commits into from
Jul 27, 2018

Conversation

Landerstraeten
Copy link
Contributor

No description provided.

@Landerstraeten Landerstraeten requested a review from veewee May 25, 2018 11:06
@Landerstraeten Landerstraeten changed the title Upgrade 7.0 Upgrade PHP 7.0 May 25, 2018
@Landerstraeten Landerstraeten changed the title Upgrade PHP 7.0 [WIP] Upgrade PHP 7.0 May 25, 2018
Copy link
Contributor

@veewee veewee left a comment

Choose a reason for hiding this comment

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

At first sight, the changes look pretty good.
As mentioned before: this PR is way too big to review. Once the comments are resolved, we will merge this one to a separate branch from where we can add more improvements.
I'll be using that branch on my projects, but I mostly use +- 8 tasks. We should find a way to validate if the changes also work on the other tasks.

This codebase is also a bit behind master ;)

@@ -34,42 +34,42 @@ function it_is_initializable()
{
$this->shouldHaveType(PhpParser::class);
}
// todo!
Copy link
Contributor

Choose a reason for hiding this comment

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

Still a todo left

ContextInterface $context,
ProcessFormatterInterface $formatter
) {
$formatter->format($process)->willReturn(Argument::type('string'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets avoid using the Argument::type(...) in a will* statement.
This only works because there is a \Prophecy\Argument\Token\TypeToken::__toString() method.
If you want to use a diferent type like integer, you'll get:

Argument::type('integer')
 notice: Object of class Prophecy\Argument\Token\TypeToken could not be converted to int

I prefer to just return a string instead. (This one is being used all over the newly introduces spec changes)

$result = $this->run($context);
$result->isPassed()->shouldBe(false);
}
// function it_should_have_configurable_options()
Copy link
Contributor

Choose a reason for hiding this comment

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

These specs should be working

$scripts[0]->shouldBe(['script.sh']);
$scripts[1]->shouldBe(['command', 'arg1', 'arg2']);
}
// todo
Copy link
Contributor

Choose a reason for hiding this comment

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

Still a todo left

$result->isPassed()->shouldBe(false);
}
// function it_does_not_do_anything_if_there_are_no_files(ProcessBuilder $processBuilder, ContextInterface $context)
// {
Copy link
Contributor

Choose a reason for hiding this comment

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

Todos

*/
public static function loadRootPackageFromJson($json, Config $config = null)
public static function loadRootPackageFromJson($json, Config $config = null): \Composer\Package\RootPackageInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid FQCN

* @return \Composer\Config
*/
public static function loadConfiguration()
public static function loadConfiguration(): \Composer\Config
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid FQCN

* @see https://secure.php.net/supported-versions.php for a list of currently supported versions
*/
public function isSupportedVersion($currentVersion)
public function isSupportedVersion(string $currentVersion): bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep the information in the docblocks

* @return bool
*/
public function isSupportedProjectVersion($currentVersion, $projectVersion)
public function isSupportedProjectVersion(string $currentVersion, string $projectVersion): bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep information in docblocks

$this->assertEquals(ParseError::TYPE_ERROR, $errors[0]->getType());
$this->assertEquals(2, $errors[0]->getLine());
}
// todo
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix todo

@Landerstraeten Landerstraeten force-pushed the upgrade-7.0 branch 2 times, most recently from 799120e to 3114418 Compare June 29, 2018 18:35
$result = $this->run($context);
$result->isPassed()->shouldBe(false);
}
// todo: need help
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@veewee I need help on this one

StephanMeijer and others added 22 commits July 9, 2018 14:02
Make detection of strict types compatible with PhpParser 4.
* Only run question helper in interactive mode

* Omit default argument
* Psalm task spec test

* Psalm task

* Added Psalm to tasks.yml

* Added more options

* Added vimeo/psalm to suggestions

* Created docs for Psalm task

* Added psalm to README.md

* Added Psalm to tasks.md

* Added description for threads parameter

* Threads is set to null by default

* Files are only added on pre-commit context

* Add newlines
@Landerstraeten Landerstraeten merged commit fdd1ce9 into phpro:grumpy-seventies Jul 27, 2018
@veewee veewee added this to the grumpy-seventies milestone Oct 25, 2018
@Landerstraeten Landerstraeten deleted the upgrade-7.0 branch December 5, 2018 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants