Upgrade to PHP 7 #651

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.

Sign up for GitHub

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

Jump to bottom

Closed

TomasVotruba wants to merge 19 commits into KnpLabs:master from TomasVotruba:php70

TomasVotruba commented Nov 16, 2017 •

edited

Loading

Replaces #647 with passing tests and CS

Why?

majority of PHP world bumping to PHP 7.1
upcoming PHP 7.2
we could also add PHP 7.1 typehints via coding standards], instead of doing that twice to just PHP 7.0
37 % people are on PHP 7.1 and growing

This package would be still available for PHP 5.6-.

Questions

1. How to approach `string`/`int` issue in parameters?

Often there was random typehint and for string and int there was autore-type to string rawurlencode method.

Should be allow both string and int and manuall rawurlencode((string) $id) or be strict in parameters?

2. How to approach `null`/`array` return values?

At least accroding to tests, some api calls return null on fail or empty results. Array was expected in typehtings.

At the moment they return array|null. Is that ok?

TomasVotruba mentioned this pull request

[WIP] Upgrade to PHP 7 #647

Closed

Contributor

m1guelpf commented Nov 16, 2017

How to approach null/array return values?

@TomasVotruba I'm not really sure, but I think in PHP7 you can use array? as a typehint, which I believe is better

Author

TomasVotruba commented Nov 16, 2017 •

edited

Loading

@m1guelpf Sure. We should move to PHP 7.1 in standalone PR, one step at a time.

This is rather architecture question, whether all these methods should return array/null

Contributor

GrahamCampbell commented Nov 18, 2017 •

edited

Loading

Sure. We should move to PHP 7.1 in standalone PR, oen step at a time.

I think that's probably a bit fast. ^7.0 will be a good version constraint through 2018 I think.

Author

TomasVotruba commented Nov 18, 2017

@GrahamCampbell My point was not in this PR. It should be PHP 7.0 only

Contributor

GrahamCampbell commented Nov 18, 2017

Sure. I was just making the point that I'd 👎 anything beyond this PR unless there were some really good reasons.

Contributor

cursedcoder commented Nov 27, 2017

What about strict types declaration in every file?
Is it possible to enforce it once? Maybe some composer flag?

Author

TomasVotruba commented Nov 27, 2017

@cursedcoder Sure, we can require that in coding standard, as I did in php-ml. But again, I'd do that in separate PR, after this one gets resolved to prevent redundant conflicts.

Collaborator

Nyholm commented Dec 12, 2017

I will tag 2.7 within a few minutes.
Could you resolve these conflicts and then merge this PR and make 2.8 our first version without PHP5.

Author

TomasVotruba commented Dec 12, 2017

Hey, I'm glad you got to this.

Sure, let me know after 2.7 is tagged

Collaborator

Nyholm commented Dec 12, 2017

It's tagged now =)

TomasVotruba added 18 commits

December 13, 2017 13:45


          use PHP 70 strict types and typehints

d1298e2


          remove unused imports and sort them

2fff0c9


          remove useless docs

0addbe7


          manual typehints fixes

fcf13c6


          fix rawurlencode string typing

52c7855


          add docblock

c5dafc7


          fix docblock comment


          travis + composer: bump to PHP 7

9a22f65


          remove useless docs

fca61a9


          type fixes

32ae3ec


          remove useless docs

6eb0c3c


          fix typehints

beb4556


          fix misstypes

fa8e633


          fix misstypes

a67c588


          fix misstypes

7d098d1


          resolve ErrorException incorrect parameter, looks like internal bug

d26af94


          fix misstypes

0c8f2a9


          remove extra spaces in docblock

fcd594c

TomasVotruba force-pushed the php70 branch from e185233 to fcd594c Compare

December 13, 2017 12:46


          fix merged tests

47685e8

Author

TomasVotruba commented Dec 13, 2017

Rebase done 👍

Author

TomasVotruba commented Dec 13, 2017 •

edited

Loading

Btw, what about bumping to PHP 7.1 in next PR? Even PHPUnit is now on board

Nyholm requested changes

View reviewed changes

Collaborator

Nyholm left a comment

Thank you. I've started to review this. I found a few issues that occur repeatedly in this PR. Could you address all of them?

lib/Github/Api/ApiInterface.php

    
                  public function getPerPage();

                  public function setPerPage($perPage);

                  public function setPerPage(int $perPage = null);

Collaborator

Nyholm Dec 13, 2017

Is this correct? Why do we allow null?

Contributor

Nek- Jan 5, 2018

This is not something new. You can specify no pagination and let github decide AFAIK. It's written as comment on some inheritance of this interface.

lib/Github/Api/Authorizations.php

    
                   * @return array

                   */

                  public function show($clientId)

                  public function show($clientId): array

Collaborator

Nyholm Dec 14, 2017

should be a string here, right?

lib/Github/Api/Authorizations.php

    
                  public function show($clientId): array

                  {

                      return $this->get('/authorizations/'.rawurlencode($clientId));

                      return $this->get('/authorizations/'.rawurlencode((string) $clientId));

Collaborator

Nyholm Dec 14, 2017

Type casting here could be removed.

lib/Github/Api/Authorizations.php

    
                   * @return array

                   * @return array|null

                   */

                  public function create(array $params, $OTPCode = null)

Collaborator

Nyholm Dec 14, 2017

$OPTCode is string, right?

lib/Github/Api/Authorizations.php

    
                  public function update($clientId, array $params)

                  {

                      return $this->patch('/authorizations/'.rawurlencode($clientId), $params);

                      return $this->patch('/authorizations/'.rawurlencode((string) $clientId), $params);

Collaborator

Nyholm Dec 14, 2017

Same here, shouldnt client ID be a string? Can you verify?

lib/Github/Api/Authorizations.php

    
                   * Reset an authorization.

                   *

                   * @param $clientId

                   * @param $token

Collaborator

Nyholm Dec 14, 2017

We should not remove these with adding type annotations.

Author

TomasVotruba Dec 14, 2017

What is the type here?

Collaborator

Nyholm Jan 4, 2018

I’m not sure without reading the code and docs.

Author

TomasVotruba commented Dec 14, 2017

Every added type was to fix tests, that failed due to strict types.

acrobat reviewed

View reviewed changes

lib/Github/Exception/ErrorException.php

    
               * @author Joseph Bielawski <stloyd@gmail.com>

               */

              class ErrorException extends \ErrorException implements ExceptionInterface

              class ErrorException extends \Exception implements ExceptionInterface

Collaborator

acrobat Dec 16, 2017

Why is this change needed?

Author

TomasVotruba Dec 16, 2017 •

edited

Loading

It's quite a long time since PR so I'm not sure.

I think it was related to mocks in some way. Try running test with/without it please to confirm.

Collaborator

acrobat commented Dec 16, 2017 •

edited

Loading

Imho this PR does a bit to much. Bumping php, adding typehints, cs fixes, etc. This makes it a bit hard for me to review the PR. Maybe we should split this up a bit.

Leave the style changes out for example as this can be done in a separate pr (I've started one with new styleci fixers, but maybe more is needed).

Bump the php version, adapt travis to it and make sure the tests still pass. After that we can start converting typehints. This way we have multiple "small" pr's which makes easy reviews and quick merges :)

This is just my opinion of course, don't know what you guys think of this.

Btw I'm definitely in favor of bumping php and thanks already for the work @TomasVotruba!

Collaborator

Nyholm commented Jan 4, 2018

I agree with acrobat.

Tomas, what is the status here?

Author

TomasVotruba commented Jan 4, 2018

What is the decision here?

Collaborator

acrobat commented Jan 5, 2018

I think we should split the pr. Leave this one for now and open a new PR where you bump the php version, adapt the travis build matrix and make sure tests still pass. From there on we can make the next steps (like adding typehints etc) in separate pr's.

Nek- reviewed

View reviewed changes

lib/Github/Api/AbstractApi.php

    
            @@ -1,4 +1,4 @@
          
              <?php

              <?php declare(strict_types=1);

Contributor

Nek- Jan 5, 2018

I don't think this is in the way of think of the project. Keeping the PHP way looks like a better alternative. Also, the indentation is wrong (it should definitely be on a new line)

Contributor

Nek- commented Jan 5, 2018 •

edited

Loading

About the first point (1. How to approach string/int issue in parameters?) the proper way to fix that is to not declare strict type hinting and type hinting string (the result is an automatic cast).

Author

TomasVotruba commented Jan 29, 2018

Not relevant anymore. I'm closing to clean my opened PRs.

Feel free to cherry pick anything you need.

TomasVotruba closed this

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

Labels

None yet