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

PHP 8.3 support added #117

Closed
wants to merge 3 commits into from
Closed

Conversation

glo71317
Copy link

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

Description

@Ocramius
Copy link
Member

I went over a few of your PRs, @glo71317: as much as I appreciate that Adobe wants PHP 8.3 support, we need builds to be green, in order for a merge to happen 😬

These patches show intent, but are of no help, if they stay red: it's just more work on @laminas/technical-steering-committee

@glo71317
Copy link
Author

I went over a few of your PRs, @glo71317: as much as I appreciate that Adobe wants PHP 8.3 support, we need builds to be green, in order for a merge to happen 😬

These patches show intent, but are of no help, if they stay red: it's just more work on @laminas/technical-steering-committee

@Ocramius completely agree :) But there is dependencies on other packages so builds are red, In order to make the build green following green build need to merge
laminas/laminas-captcha#31
laminas/laminas-code#194
laminas/laminas-loader#12
laminas/laminas-uri#21
laminas/laminas-json#18
laminas/laminas-math#19
laminas/laminas-recaptcha#33
laminas/laminas-soap#42
laminas/laminas-text#62
laminas/laminas-cache-storage-adapter-memory#53

If you have any pr comments then feel free leave the comments we will resolve the pr comments asap else proceed for merge above prs to speed up to make green other prs :)

@Ocramius
Copy link
Member

Yes, but the current effect is that I have a huge load of emails, and I'm just playing whack-a-mole with them.

That's why I'm saying it isn't helpful 😁

I will go through the list from your comment though 👍

@Ocramius
Copy link
Member

Meanwhile, all mentioned patches should have associated releases 👍

@boesing
Copy link
Member

boesing commented Oct 18, 2023

For those packages where transitive dependencies are not yet supporting PHP 8.3 and therefore the checks are not passing, you can add ignore_php_platform_requirements to .laminas-ci.json in project root.

{
     "ignore_php_platform_requirements": {
        "8.3": true
     }
}

If checks are still failing afterwards, that might be related to transitive dependencies not supporting PHP 8.3.
In case these are laminas components, this might be raised in those components.

In any case, we have to wait until there are patched versions for those components which lead to failing tests due to the PHP update.

@glo71317
Copy link
Author

@Ocramius I have updated all associated release for patches and tests are green now.
Can you review and merge this pr if there is no concern?

@glo71317
Copy link
Author

glo71317 commented Nov 7, 2023

@Ocramius @gsteel PR is ready more than a week, No one PR is reviewing?

Copy link
Member

@gsteel gsteel left a comment

Choose a reason for hiding this comment

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

Thanks @glo71317 - There are a few easily solved but baselined issues here that have been handled in #118 - so I'll prefer that patch over this one.

Comment on lines +375 to +377
<DeprecatedMethod occurrences="5">
<code>$this->returnValue</code>
</DeprecatedMethod>
Copy link
Member

Choose a reason for hiding this comment

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

These deprecations can be fixed in the test with MockObject::willReturn($whatever)

Comment on lines +534 to +546
<DeprecatedMethod occurrences="2">
<code>$this->returnValue</code>
</DeprecatedMethod>
</file>
<file src="test/HydratorTestTrait.php">
<DeprecatedMethod occurrences="4">
<code>$this->returnValue</code>
</DeprecatedMethod>
</file>
<file src="test/NamingStrategy/CompositeNamingStrategyTest.php">
<DeprecatedMethod occurrences="4">
<code>$this->returnValue</code>
</DeprecatedMethod>
Copy link
Member

Choose a reason for hiding this comment

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

These deprecations can be fixed in the test with MockObject::willReturn($whatever)

Comment on lines +175 to +177
<ArgumentTypeCoercion>
<code>$pattern</code>
</ArgumentTypeCoercion>
Copy link
Member

Choose a reason for hiding this comment

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

This can be solved by asserting that preg_match receives a non-empty-string

Comment on lines +214 to +216
<ArgumentTypeCoercion>
<code>$pcreInfo->pattern</code>
</ArgumentTypeCoercion>
Copy link
Member

Choose a reason for hiding this comment

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

This can be solved by asserting that preg_match receives a non-empty-string

@gsteel gsteel self-assigned this Nov 8, 2023
@gsteel gsteel added the Duplicate This issue or pull request already exists label Nov 8, 2023
@gsteel
Copy link
Member

gsteel commented Nov 8, 2023

Handled in #118

@gsteel gsteel closed this Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate This issue or pull request already exists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants