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

Added PHP 8.2 support, dropped PHP 7 support #12

Merged
merged 21 commits into from
Dec 11, 2022

Conversation

glo71317
Copy link
Contributor

@glo71317 glo71317 commented Dec 5, 2022

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

.laminas-ci.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
@Ocramius
Copy link
Member

Ocramius commented Dec 5, 2022

Hmm, CI failures highlights some needs for changes.

@Ocramius
Copy link
Member

Ocramius commented Dec 6, 2022

The test failures need to be addressed, not worked around 😁

@Ocramius
Copy link
Member

Ocramius commented Dec 7, 2022

I think the minimum PHPUnit version needs to be raised to 9.5 here

@glo82145
Copy link
Contributor

glo82145 commented Dec 7, 2022

I think the minimum PHPUnit version needs to be raised to 9.5 here

@Ocramius Thank you so much , this solution has worked here. Still One test case is failing, looking into that

@glo82145
Copy link
Contributor

glo82145 commented Dec 8, 2022

@Ocramius there is one testcase failing due to squizlabs/php_codesniffer. It could be due to same reason as you mentioned in PR laminas/laminas-oauth#20. Can you please confirm?

@Ocramius
Copy link
Member

Ocramius commented Dec 8, 2022

I wonder if we should just skip PHPCS: we don't want to improve CS on these components, as it poses more risk (regressions).

Alternatively, upgrading of laminas/laminas-coding-standard is possible, but with very conservative changes.

@glo82145
Copy link
Contributor

glo82145 commented Dec 8, 2022

I wonder if we should just skip PHPCS: we don't want to improve CS on these components, as it poses more risk (regressions).

Alternatively, upgrading of laminas/laminas-coding-standard is possible, but with very conservative changes.

@Ocramius Yes, upgrading laminas/laminas-coding-standard would ask for lot of changes which will make things more complicated at this stage. Hence it should be investigated further to find appropriate solution. But as it is blocking this current PR from merging and creating a blocker, Can we just skip this one from this PR and take it up as different ticket for further investigation..

@Ocramius
Copy link
Member

Ocramius commented Dec 8, 2022

But as it is blocking this current PR from merging and creating a blocker

There is no blocker over here? Simply means it won't be merged until green here.

Can we just skip this one from this PR and take it up as different ticket for further investigation..

IMO no: merging broken stuff is just going to lead to more load on the maintainers: we don't leave stuff half-done here, as it's mostly making thing complicated for everyone else.

phpcs.xml Outdated
@@ -17,24 +17,4 @@
<file>test</file>
<exclude-pattern>*/_files/*</exclude-pattern>
<exclude-pattern>*/TestAsset/*</exclude-pattern>

<!-- Include all rules from Laminas Coding Standard -->
<rule ref="LaminasCodingStandard">
Copy link
Member

Choose a reason for hiding this comment

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

The LaminasCodingStandard should still be preserved

@glo82145
Copy link
Contributor

glo82145 commented Dec 9, 2022

@Ocramius We have upgraded the laminas/laminas-coding-standard and updated the code as per the requirement.

@Ocramius Ocramius assigned Ocramius and unassigned glo71317 Dec 11, 2022
@Ocramius Ocramius changed the title Added php8.2 support Added PHP 8.2 support, dropped PHP 7 support Dec 11, 2022
glo71317 and others added 5 commits December 11, 2022 15:41
Strict types are too problematic to enable in a minor release: we
will therefore disable the CS rule that enforces them to be declared.

This change also reverts the previous addition of strict types to
sources that can crash due to types mismatching internally.
…debase

This codebase is tested, but is old and full of quirks: including static analysis
tooling will prevent minor changes from accidentally breaking it over minor details.
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.

I had to rewrite the patch to fix multiple changes done incorrectly, such as removed @throws descriptions, incorrect/wrong @var annotations and removed @deprecated comments.

I also squashed previous commits, which were just saying mentioning that some issues were being fixed, without any mention if which they were.

In practice, I understand that you were assigned this task (outsourced?), and that you had no experience with this, but I really wish you had put some thinking into the errors that the CS tooling reported, rather than just steamrolling over the pre-existing code.

Instead, it seems like you've bashed your keyboard until you reached green, which is a bit sad, since it just means double work for maintainers :-(

Anyway, this is now ready for release.

@Ocramius Ocramius merged commit 40f7acd into laminas:2.10.x Dec 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants