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

Fix/php81 #44

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Fix/php81 #44

wants to merge 15 commits into from

Conversation

Donatello-za
Copy link

Hi, I am concerned about the future of this repository and through this pull request I'm hoping to get the library up to PHP 8.1's standard and compatibility.

Some notes:

  • These changes are not backwards compatible with PHP versions older than 8.1.
  • The changes made with this un-merged pull request GH-43 was also implemented.
  • You'll have to consider strategies for how to allow upgrades from the 2.x branch or the 3.x branch.
  • Every file apart from the unit tests have been worked on. This was to fully implement strong typing and PHP 8.1 features.
  • The deprecated LogFormatter class and unit tests have been removed.
  • Unit Testing success is 100% and coverage is the same as before. However, the dev dependencies of the external vanilla/garden-container repository causes deprecation warnings during unit testing. To solve this, that package will also need upgrading to PHP 8.1
  • Psalm analyses succeeds 100% with 0 warnings or notices.

If you end up using this pull request your welcome to squash the commits, I can also do it from this side but I'm not sure if you'd like to see the progression of the commits.

@Donatello-za
Copy link
Author

I did notice just now that some source code formatting changes crept in due to my IDE setup. If you are not happy with it I can change those back.

@blacktek
Copy link

blacktek commented Nov 1, 2022

Hello,
any idea when the pull request of Donatello-za can be accepted and the new package released with composer?

@CLAassistant
Copy link

CLAassistant commented Dec 1, 2022

CLA assistant check
All committers have signed the CLA.

@@ -11,7 +11,7 @@
"require": {
Copy link

Choose a reason for hiding this comment

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

I would suggest to use a require for "php 7.4" as the code is basically compatible with 7.4

Copy link
Author

Choose a reason for hiding this comment

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

Hi @blacktek, unfortunately it is not backwards compatible with PHP 7.4 due to keywords such mixed being used as parameters or return types amongst other things.

Copy link

Choose a reason for hiding this comment

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

I'm currently using it on 7.4 too and is working. 7.4 doesn't support multiple union return types (e.g. string | bool), that I remember were not used. Are you sure that those are currently used?

Copy link
Author

Choose a reason for hiding this comment

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

I assumed there would be problems since the psalm test fails but if these warnings/errors can be fixed quickly in order to return composer.json to 7.4 it may be worth it. I will look into it when I have some time.

Copy link
Author

Choose a reason for hiding this comment

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

I had a quick look but I'm guessing you are not using my Fix/php81 fork in PHP 7.4. My fork uses things like the mixed keyword as function parameters and function return types. Many switch statements were replaced by match statements. Many methods returns the static keyword. The throw new statement is used. Union types are used as function arguments, and so on.

My fork is strictly for PHP 8.1 which is why I asked the repository owner whether two separate branches can be released, i.e. a new 4.x branch alongside the 3.x branch.

Alternatively they could just resurrect their repository but it seems the primary contributor is no longer available and active on Github and have not been for a year.

Copy link

Choose a reason for hiding this comment

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

it's possible that you're right. I'm struggling a bit using your Fix/php81 on php8.1 and probably you're right that I use the old dev-master on php7.4

tnx

Copy link
Author

Choose a reason for hiding this comment

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

If you are struggling to set up my fork in composer.json here is an extract you can use:

{
  "require": {
    "php": "^8.1",
    "vanilla/garden-cli": "dev-fix/php81"
  },
  "repositories": [
    { "type": "vcs", "url":  "https://github.com/donatello-za/garden-cli" }
  ]
}

Copy link

Choose a reason for hiding this comment

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

tnx a lot :) this is exactly what I was using.

The OptSchema::merge() function may create an array of strings if a "main"/"global" description is supplied as well as a description for individual commands. This fix will prevent errors in such a case and will simply show the correct description for the command.
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.

4 participants