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

Ugrade packages and add PHP 8 Support #203

Merged
merged 2 commits into from Jan 4, 2022
Merged

Conversation

merlindiavova
Copy link
Contributor

This PR addresses a few issues regarding exceptions thrown by get_parent_class and ReflectionClass.

As get_parent_class throws an error if the class names is malformed and/or does not exist, I have added a class_exists check before passing the class name to get_parent_class

This PR does not introduce any breaking changes. Although PHP 8 is support it uses nothing PHP 8 specific.

@frederikbosch
Copy link
Contributor

Perfect! I will spend time on merging this next week.

@kenjis
Copy link
Member

kenjis commented Jul 27, 2021

This PR must go into 5.x branch (it does not exist now).

But it seems this changes could run on PHP 7.2 or later, so it is better to keep the major version at 4.
The Aura 4.x series is for PHP 7.2 and later.

For the 5.x series, PHP 8.0 (or 8.1) or later is a good choice. It is easy to remember.

See auraphp/Aura.Cli_Kernel#12 (comment)

  • The Aura 3.x series was for PHP 5.6 and later.
  • The Aura 4.x series is for PHP 7.2 and later.
  • I imagine a 5.x series, if you want to start one, should be for 8.0 (or perhaps 8.1 ?) and later.

@merlindiavova
Copy link
Contributor Author

Hey, any news/movement on this.

Really could use this in my projects moving forward.

Thanks

@kenjis
Copy link
Member

kenjis commented Dec 12, 2021

If this goes 4.x, PHP 7.2 and later.
If this goes 5.x, PHP 8.1 and later.

auraphp/Aura.Sql#199 (comment)

  • The Aura 3.x series was for PHP 5.6 and later.
  • The Aura 4.x series is for PHP 7.2 and later.
  • The Aura 5.x series is for PHP 8.1 and later.

Copy link
Contributor

@frederikbosch frederikbosch left a comment

Choose a reason for hiding this comment

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

This took way too long. Thanks a lot for your PR. I left some remarks. Some are questions to learn why something is changed. Hopefully we can get this released soon!

@@ -206,7 +208,7 @@ public function getUnified(string $class): Blueprint
}

// fetch the values for parents so we can inherit them
$parent = get_parent_class($class);
$parent = class_exists($class) ? get_parent_class($class) : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason for this extra call?

@@ -96,7 +97,7 @@ function ($val) {

return $val;
},
$this->params
array_values($this->params)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is there reason for this extra call? Arrays are already sorted, right?

throw $re;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Why would we want to catch that ReflectionException? What is the use-case for that?


return false;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We could consider making this part of the package itself, rather than only tests. I don't know if would have any value though.

@frederikbosch frederikbosch merged commit 490d251 into auraphp:4.x Jan 4, 2022
@frederikbosch
Copy link
Contributor

@merlindiavova Thank you very much for this contribution. I added Github actions to ensure our CI process is solid again. Also just tagged 4.2.0 with the support for PHP 8.0.

@merlindiavova
Copy link
Contributor Author

@frederikbosch I totally missed this! Sorry you had to finish it off! Glad its in nevertheless!

@@ -18,18 +18,18 @@
}
],
"require": {
"php": ">=7.2.0",
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @kenjis as 7.2 was already there we should not change the php version. If phpunit was the reason switching to 7.4 we should use yoast polyfill.

@harikt harikt mentioned this pull request Feb 6, 2022
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.

None yet

6 participants