Skip to content

Conversation

@kukulich
Copy link
Contributor

No description provided.

@enumag
Copy link
Contributor

enumag commented Aug 14, 2015

Maybe the param and return annotations should be removed as they're pretty much duplicate.

@kukulich
Copy link
Contributor Author

I agree with you. I've prepared this principally to discuss how the PHP7 pull requests should look.

@Majkl578
Copy link
Contributor

how the PHP7 pull requests should look

What do you mean? PHP 7 changes are backward incompatible and there is no way to make it cross-compatible with PHP 5.x. The only way seems to be some sort of converter like we had with 5.2 vs. 5.3. This would be PITA.

Maybe the param and return annotations should be removed as they're pretty much duplicate.

They are not always, because PHP 7 still does not support nullable types. Also Nette heavily uses mixed, sadly.

@kukulich
Copy link
Contributor Author

What do you mean? PHP 7 changes are backward incompatible and there is no way to make it cross-compatible with PHP 5.x. The only way seems to be some sort of converter like we had with 5.2 vs. 5.3. This would be PITA.

Exactly. So there has to be some discussion if PHP 7 features will be in new branch, in master branch but in comments or there will be some sort of converter.

There should probably be special branch for PHP 7 because of composer. Version 2.4 can be PHP 5.4 compatible, version 3.0 for PHP 7+.

@JanTvrdik
Copy link
Contributor

👎 Pointless, premature. Author does not give any arguments why should this PR be of any value. Discussion should be done on forum not on random Nette repository in PR.

@kukulich
Copy link
Contributor Author

@dg
Copy link
Member

dg commented Aug 15, 2015

The complicated part is that now you have to rebase this PR until Nette goes to PHP 7.

@dg
Copy link
Member

dg commented Aug 15, 2015

Btw, interesting thing is that return self works as static and static is parse error http://3v4l.org/TvJjC. This is the end of discussion about @return self vs static.

Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The parameter should be integer, string jpg throws TypeError. However I can add another test for the TypeError exception.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I understand

@kukulich
Copy link
Contributor Author

The complicated part is that now you have to rebase this PR until Nette goes to PHP 7.

Yes. However I think there are plenty of projects that would use PHP 7 Nette (eg. Slevomat, Rohlik, DameJidlo) so there should be enough people to do the rebase.

@kukulich
Copy link
Contributor Author

I will prepare another pull request with changes that could be merged to master so the rebase would be easier then.

@JanTvrdik
Copy link
Contributor

The rebasing has to be fully or at least partially automated. Doing it manually would by boring and error-prone. @kukulich Do you have time to work on such tool?

@kukulich
Copy link
Contributor Author

There's already tool doing the opposite: https://github.com/ondrejbouda/php7backport

So if we move master to PHP 7, we can rebase another branch to be PHP 5.4 compatible. It would be much easier.

I think we can prepare automatic tool during our hackathons in Slevomat.

@JanTvrdik
Copy link
Contributor

So if we move master to PHP 7

Not going to happen any time soon. Also the tool is currently missing a lot of conversions and the conversions that are present are often not implemented properly.

@dg
Copy link
Member

dg commented Aug 15, 2015

To move Nette to PHP 7 means

  • move it to PHP 5.5 and to 5.6 (see end of https://phpfashion.com/jak-se-opousti-php-5-3)
  • have tools like ApiGen ready for PHP 7 syntax
  • have the ability to widely test it on PHP 7 (it means do it close to the time when PHP7 will be released)

@kukulich
Copy link
Contributor Author

PHP 7 is not released so it's time to improve the tool :)

I think it doesn't matter if master is in PHP 7 or not. It just depends how the release process works.

@dg dg force-pushed the master branch 13 times, most recently from 6c03b1f to a4bddc8 Compare September 9, 2015 07:16
@dg dg force-pushed the master branch 9 times, most recently from d6db352 to 02ddff7 Compare December 23, 2016 03:29
@dg dg force-pushed the master branch 3 times, most recently from d283776 to d8eadeb Compare January 4, 2017 14:24
@dg dg force-pushed the master branch 12 times, most recently from d116328 to 3054b70 Compare January 15, 2017 05:55
@kukulich kukulich closed this Jan 16, 2017
@dg
Copy link
Member

dg commented Jan 20, 2017

@kukulich Do you have any tool to generate typehints? And could you send PR to other packages too?

@kukulich
Copy link
Contributor Author

@dg It should be possible with this: https://github.com/slevomat/coding-standard/blob/2.0-dev/SlevomatCodingStandard/Sniffs/TypeHints/TypeHintDeclarationSniff.php

There's no documentation but I can prepare a manual for you how to use it next week.

@dg
Copy link
Member

dg commented Jan 20, 2017

Great!

@kukulich
Copy link
Contributor Author

kukulich commented Jan 21, 2017

@dg
Copy link
Member

dg commented Jan 21, 2017

Thanks :-)

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.

5 participants