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 #62

Merged
merged 6 commits into from
Nov 7, 2023
Merged

PHP 8.3 support added #62

merged 6 commits into from
Nov 7, 2023

Conversation

glo71317
Copy link
Contributor

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

Description

@glo71317
Copy link
Contributor Author

@froschdesign Can you please review the PR? We are looking for your help to merge this because we are working on Magento Dependencies and support php 8.3 due to laminas dependencies we are blocked so looking forward from your side. Can you please do the needful?

@@ -832,6 +832,10 @@ protected function _smushAmount()
if (empty($leftChar) || $leftChar === ' ') {
$amount++;
} elseif (! empty($rightChar)) {
/**
* @psalm-suppress RedundantConditionGivenDocblockType
Copy link
Member

Choose a reason for hiding this comment

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

We should baseline these, not suppress them

Copy link
Contributor

Choose a reason for hiding this comment

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

Made all the suggested changes and pushed the code. Kindly review. Thank you.

/** @var FactoriesConfigurationType */
/**
* @var FactoriesConfigurationType
* @psalm-suppress NonInvariantDocblockPropertyType
Copy link
Member

Choose a reason for hiding this comment

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

This is fixable: doesn't need suppression

Copy link
Contributor

@glo17680 glo17680 Oct 31, 2023

Choose a reason for hiding this comment

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

Done

@@ -499,6 +499,7 @@ public function render()
/**
* Magic method which returns the rendered table
*
* @psalm-suppress MethodSignatureMustProvideReturnType
Copy link
Member

Choose a reason for hiding this comment

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

This should be baselined, not suppressed

Copy link
Contributor

@glo17680 glo17680 Oct 31, 2023

Choose a reason for hiding this comment

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

Done

@@ -9,6 +9,7 @@

/**
* @group Laminas_Text
* @psalm-suppress DeprecatedMethod
Copy link
Member

Choose a reason for hiding this comment

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

To be baselined, not suppressed

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

@glo71317
Copy link
Contributor Author

glo71317 commented Nov 7, 2023

@Ocramius @gsteel PR is ready more than a week, No one PR is reviewing Can you please help us to review and merge this?

@Ocramius Ocramius self-assigned this Nov 7, 2023
@Ocramius Ocramius added this to the 2.11.0 milestone Nov 7, 2023
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.

LGTM

About timeline: nobody (that I know of) is paid to work on this over here, and we do it in our free time/when we can/out of our good will, so please be patient, as our only timeline is "when we want to do it between work and life" :P

@Ocramius Ocramius merged commit 20ff153 into laminas:2.11.x Nov 7, 2023
10 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants