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

Upgrade iguana for PHP 8 compatibility #46

Merged
merged 6 commits into from
Sep 9, 2024

Conversation

patdel0
Copy link
Contributor

@patdel0 patdel0 commented Jun 28, 2024

This PR resolves the deprecation warning, by upgrading iguana to the latest version:

Return type of Dxw\Iguana\Value\ArrayBase::offsetUnset($offset) should either be compatible with ArrayAccess::offsetUnset(mixed $offset): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice

How to Test

  1. Install and activate this plugin
  2. Navigate to wp-content/plugins/govuk-components-plugin
  3. Checkout this branch chore/update-composer-dependencies
  4. Confirm the deprecation warning is no longer present

@patdel0 patdel0 force-pushed the chore/update-composer-dependencies branch 3 times, most recently from 6c9d488 to 5a10079 Compare June 28, 2024 12:32
@patdel0 patdel0 marked this pull request as ready for review June 28, 2024 12:32
Copy link
Contributor

@jkeasley jkeasley left a comment

Choose a reason for hiding this comment

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

LGTM - can you check why the unit tests and php-cs-fixer aren't completing before merging though

@snim2
Copy link
Contributor

snim2 commented Jul 1, 2024

LGTM - can you check why the unit tests and php-cs-fixer aren't completing before merging though

I notice these are for PHP7.2 which we no longer deploy. We should probably add a PHP 8 to the matrix and remove 7.2

@patdel0
Copy link
Contributor Author

patdel0 commented Jul 1, 2024

@jkeasley The 7.2 checks have been removed from the workflow, hence why they aren't running (?). I expect that once the revised workflow is integrated, these checks will no longer be part of future merges/pushes.
5a10079
Is my assumption incorrect?

@snim2
Copy link
Contributor

snim2 commented Jul 1, 2024

@jkeasley The 7.2 checks have been removed from the workflow, hence why they aren't running (?). I expect that once the revised workflow is integrated, these checks will no longer be part of future merges/pushes. 5a10079 Is my assumption incorrect?

Ah, they are listed in some branch protection rules, so I'll remove them, but we should still add a PHP8 to the matrix (then I'll add those rules in)

composer.json Outdated
@@ -1,7 +1,7 @@
{
"config": {
"platform": {
"php": "7.4"
"php": "8.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about this TBH. There are two places where we can add the platform, one where you can use || and one where you cannot. What happens here if we try to run composer install in 7.4?

Copy link
Contributor Author

@patdel0 patdel0 Jul 9, 2024

Choose a reason for hiding this comment

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

Sorry, I thought I replied to this already.

It seems to work okay on my end when testing it on saluki 7.4.
Regardless, I changed it to match what we did in iguana

@snim2
Copy link
Contributor

snim2 commented Jul 1, 2024

Since this is a library package, and intended to be installed as a dependency, I would not expect to have a composer.lock file in the repo (I'd expect it to be in .gitignore). @jkeasley that's our usual convention, right?

@jkeasley
Copy link
Contributor

jkeasley commented Jul 1, 2024

@snim2 yeah, I'd expect the composer.lock to be in .git-ignore

@patdel0
Copy link
Contributor Author

patdel0 commented Jul 9, 2024

@snim2 Would you like me to rebase/resolve the conflicts before giving it another look?

@patdel0 patdel0 requested a review from snim2 July 9, 2024 16:09
@serena-piccioni serena-piccioni force-pushed the chore/update-composer-dependencies branch from d0efa0d to ecff466 Compare September 9, 2024 11:01
@serena-piccioni serena-piccioni force-pushed the chore/update-composer-dependencies branch from ecff466 to fe07f50 Compare September 9, 2024 11:05
@serena-piccioni serena-piccioni merged commit 016b2d0 into main Sep 9, 2024
5 checks passed
@serena-piccioni serena-piccioni deleted the chore/update-composer-dependencies branch September 9, 2024 11:06
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