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

Remove mixed type to avoid PHP 8.0+ features #9

Closed
josemv92 opened this issue Jan 15, 2022 · 5 comments
Closed

Remove mixed type to avoid PHP 8.0+ features #9

josemv92 opened this issue Jan 15, 2022 · 5 comments

Comments

@josemv92
Copy link
Contributor

Since in some of the cases this will be used to do the transition from older PHP versions, it will be better for the changes done to not use features included only in PHP8 or above.

One example of this is the mixed type, which was introduced on PHP 8, which is now being used in some of the methods like here

The reason for this request is to prevent adding changes which will force developers into PHP 8, since this is mostly going to help developers transitioning from an older version while they prepare the complete application. These covers scenarios such as:

  • Running the upgrade project in a pre-PHP 8 server for testing.
  • Having a release of the code on any server running an older version before the PHP upgrade.

It could benefit developers to be able to use this with a version prior to PHP 8 in case the applications they have to migrate are huge and could take time to be fully done, but they want to start testing with the new version.

@kamilwylegala
Copy link
Owner

Hey @josemv92

Thanks for raising this issue.

I decided to start using PHP 8.0 features in this fork because I didn't have issues running original framework on PHP 7.*.

Transition from PHP 7.0 to 7.4 was smooth. Only in PHP 8 there appeared blockers so fork was a must have. I added mixed type recently to avoid deprecation notices in PHP 8.1

One important thing to mention: I stopped using CakePHP's "test classes" to test my application. Thanks to that I was able to upgrade PHPUnit first and then start upgrading PHP.

Unfortunately old PHPUnit is very coupled to the framework - I did some effort already to upgrade required version in the framework but it's gonna be long process. I also emphasized it in the readme file. I trust mostly code coverage of my project when I migrate things.

In my opinion migration should look like this:

  • get rid of usage of CakeTestCase in your code
  • upgrade PHP Unit to meet platform requirements in composer.json
  • migrate your project gradually to PHP 7.4 through all 7.* versions
  • once it's fine on PHP 7.4, switch to PHP 8 along with this fork

Let me know what you think :)

@josemv92
Copy link
Contributor Author

Agreed on the test cases, it's still something I'm trying to figured out as well.

The problem I see with using PHP 8+ features, and as you stated, is that it requires to migrate into the new PHP version which forces to release these changes with an upgrade of PHP 8+, which will potentially mean a downtime of the applications which in some cases might not be acceptable. Or also to cover cases where it might not be possible to do the migration and upgrade together (for example on teams which handle multiple apps or multiple environments).

Not sure if having those deprecation messages will be bad but if the goal on this fork will be to solve all of them even if they require PHP 8+ features, it might be worth mentioning on the readme this fork will not work with versions prior to PHP 8, so it is clear it is not just a patched version of the framework which will continue working with what the framework originally supported as well.

This might not be too huge of a deal, having the framework live for a longer time is an amazing work (and thanks for that). I just brought up this since some people might want to follow a different release approach and if this requires to do the upgrade to 8, it might be worth mentioning somewhere so it is clear beforehand.

Let me know your thoughts on it.

@kamilwylegala
Copy link
Owner

Thanks for your input. You're right. Intention of this fork is not explained in the readme. I just assumed that people want to migrate to PHP 8 and have no limitation on the framework.

My overall goal is to maintain this fork as long as new PHP versions are going to be released.

To sum up:

  • Since CakePHP is fine on PHP 7.0-7.4, I will emphasize in the readme that original "cakephp/cakephp": "^2.0.0" should be used
  • I'll explain what's the goal of the fork (having PHP8) in readme and how migration process should look like. I'll also leave a note on what upgrades will be provided to the fork.

Sounds good? If so, I will create merge request to the readme file.

@josemv92
Copy link
Contributor Author

That works, ideally everyone should be aiming to move to a PHP 8+ version as soon as possible since latest 7.x will end support this year, just thinking in some scenarios (especially for some companies) where this could not be as easy to do it this way.

Having a clear message on what the fork aims for could warn users for knowing those details. I believe if there are some specific use cases they could do their own fork as needed.

@kamilwylegala
Copy link
Owner

I updated the readme file. Thanks for your help @josemv92 !

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

No branches or pull requests

2 participants