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

Drop PHP 7, add PHP 8.2 Support #96

Merged
merged 11 commits into from
Jul 11, 2023

Conversation

rogervila
Copy link
Contributor

Q A
Documentation no
Bugfix no
BC Break no
New Feature no
RFC no
QA no

Description

Support PHP 8.2

@rogervila rogervila marked this pull request as draft December 20, 2022 09:03
@rogervila
Copy link
Contributor Author

rogervila commented Dec 20, 2022

Needs laminas-api-tools/api-tools-api-problem to be upgraded (laminas-api-tools/api-tools-api-problem#24)

@froschdesign froschdesign added the Enhancement New feature or request label Dec 20, 2022
composer.json Outdated
@@ -29,7 +29,7 @@
}
},
"require": {
"php": "^7.3 || ~8.0.0 || ~8.1.0",
"php": "^7.3 || ~8.0.0 || ~8.1.0 || ~8.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.

Needs composer.lock update too - possibly good idea to drop PHP 7 to simplify the matrix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@rogervila rogervila marked this pull request as ready for review January 5, 2023 08:41
@rogervila rogervila marked this pull request as draft January 5, 2023 08:41
@rogervila rogervila marked this pull request as ready for review January 10, 2023 09:47
@rogervila rogervila changed the title PHP 8.2 Support Drop PHP 7, add PHP 8.2 Support Jan 10, 2023
@rogervila rogervila marked this pull request as draft January 10, 2023 09:51
@rogervila
Copy link
Contributor Author

Needs laminas-api-tools/api-tools-content-negotiation to support PHP 8.2

@rogervila
Copy link
Contributor Author

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Provided an initial review pass: it would be good to keep a larger baseline at first.

Patch is OK otherwise

psalm.xml.dist Show resolved Hide resolved
src/DbConnectedResource.php Show resolved Hide resolved
@@ -97,6 +98,7 @@ public function fetch($id)
if (0 === $resultSet->count()) {
throw new DomainException('Item not found', 404);
}
/** @psalm-var array|object */
Copy link
Member

Choose a reason for hiding this comment

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

Same here: let's leave the baseline larger here, rather than providing inline hints

Copy link
Contributor

Choose a reason for hiding this comment

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

Done.

@@ -128,6 +130,7 @@ protected function retrieveData($data)
return $filter->getValues();
}

/** @psalm-var array<string, mixed> */
Copy link
Member

Choose a reason for hiding this comment

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

Isn't getValues() already in this shape? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

It is. I've removed the hint, though this means stuffing something into the baseline. But Psalm should have been able to infer this.

@@ -22,6 +23,12 @@ class ApplicationTest extends TestCase
{
use ProphecyTrait;

/** @var ContainerInterface|ObjectProphecy */
Copy link
Member

Choose a reason for hiding this comment

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

We can use native types in here, if needed, otherwise all good, so this is just a nit

Copy link
Contributor

Choose a reason for hiding this comment

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

Done.

Bumps to versions known to test against 8.2.

Signed-off-by: Matthew Weier O'Phinney <[email protected]>
Turns out, there's no need anyways.

Signed-off-by: Matthew Weier O'Phinney <[email protected]>
Per @Ocramius

Signed-off-by: Matthew Weier O'Phinney <[email protected]>
@weierophinney
Copy link
Contributor

weierophinney commented Jul 10, 2023

Currently blocked by laminas-api-tools/api-tools-content-validation#22; turning to that next.

Signed-off-by: Matthew Weier O'Phinney <[email protected]>
@weierophinney
Copy link
Contributor

Now blocked by api-tools-mvc-auth, turning to that.

Signed-off-by: Matthew Weier O'Phinney <[email protected]>
- Bumps rpc to 1.8
- Bumps servicemanager to 3.11+
- Drops laminas-zendframework-bridge

Signed-off-by: Matthew Weier O'Phinney <[email protected]>
Picks up PHP 8.2 support

Signed-off-by: Matthew Weier O'Phinney <[email protected]>
@weierophinney weierophinney marked this pull request as ready for review July 11, 2023 18:29
@weierophinney weierophinney added this to the 1.7.0 milestone Jul 11, 2023
@weierophinney
Copy link
Contributor

Ready for final review!

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

@Ocramius Ocramius self-assigned this Jul 11, 2023
@Ocramius Ocramius merged commit 5ecdfc1 into laminas-api-tools:1.7.x Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants