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.

Sign up for GitHub

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

Jump to bottom

PHP8.1 support added #81

Merged
merged 5 commits into from
Oct 12, 2021
Merged

PHP8.1 support added #81

merged 5 commits into from
Oct 12, 2021

Conversation

konarshankar07
Copy link
Contributor

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

Description

PHP8.1 support added

Related ticket: magento/magento2#34214

@froschdesign froschdesign added Enhancement hacktoberfest-accepted Issues/Pull-Requests which can be fixed during Hacktoberfest: https://hacktoberfest.digitalocean.com labels Oct 12, 2021
@froschdesign froschdesign added this to the 2.13.0 milestone Oct 12, 2021
@konarshankar07 konarshankar07 marked this pull request as draft October 12, 2021 06:45
@weierophinney
Copy link
Member

@konarshankar07 I've added a few commits to get this working on latest deps and with 8.1; thanks for getting this started!

@froschdesign
Copy link
Member

@weierophinney
There is a separate pull request for the Markdown problems: #78

@weierophinney
Copy link
Member

There is a separate pull request for the Markdown problems: #78

I've just pushed a fix for them here, as I'd already started before you commented. :-/

Not entirely sure why it ran those checks, TBH - there were no changes to the docs, so those should not have run.

@weierophinney weierophinney marked this pull request as ready for review October 12, 2021 16:41
@weierophinney weierophinney changed the base branch from 2.12.x to 2.13.x October 12, 2021 16:51
konarshankar07 and others added 5 commits October 12, 2021 11:51
@konarshankar07 @weierophinney
PHP8.1 support added
cef4b9f
@weierophinney
feat: update minimum supported versions of dependencies
10a59f6 
Use versions known to work with PHP 8.1 where possible.

Update how package replaces zend-view:

- Make a conflict with all versions of zend-view
- Remove "replace" entry for zend-view
- Remove requirement on laminas-zendframework-bridge

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
@weierophinney
qa: convert deprecations to exceptions
40bc076 
Restore pre-9.5.10 behavior of PHPUnit whereby it converted deprecations to exceptions, as we have tests that were expecting deprecation notices.

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
@weierophinney
fix: ensure code works under PHP 8.1 without deprecation warnings
6c73cf8 
- Adds polyfill classes to provide PHP 8.1-compliant versions of classes where components are not yet updated:
  - `Laminas\Filter\FilterChain`
  - `Laminas\Mvc\Plugin\FlashMessenger`
  - `Laminas\Navigation\AbstractContainer`
- Adds `ReturnTypeWillChange` attributes to interface method implementations.
- Ensures that `md5()` is only ever passed strings.

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
@weierophinney
qa: ignore platform PHP requirement on PHP 8.1 runs
1d7fb06 
Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
@weierophinney weierophinney merged commit 1d8d10b into laminas:2.13.x Oct 12, 2021
@froschdesign froschdesign mentioned this pull request Oct 12, 2021
Closed
@github-actions github-actions bot mentioned this pull request Oct 12, 2021
Merged
@fascinosum
Copy link

hi @weierophinney, thank you and @konarshankar07 for the implemented support of PHP 8.1

@weierophinney, could you explain the reason for

laminas-view/composer.json

Line 58 in 2b396ad

"zendframework/zend-router": "*"

If project has laminas/laminas-zendframework-bridge as dependency, it does not allow this version to be installed, because conflicts with zendframework/zend-router * (laminas/laminas-router 3.4.5 replaces zendframework/zend-router ^3.3.0)
You can check it with the latest available version of laminas/laminas-mvc

@fascinosum fascinosum mentioned this pull request Oct 12, 2021
Closed
@weierophinney
Copy link
Member

If project has laminas/laminas-zendframework-bridge as dependency, it does not allow this version to be installed, because conflicts with zendframework/zend-router * (laminas/laminas-router 3.4.5 replaces zendframework/zend-router ^3.3.0)
You can check it with the latest available version of laminas/laminas-mvc

Oof - that was an incorrect line - it should be zend-view. I'll get that updated and issue a patch release.

weierophinney added a commit to weierophinney/laminas-view that referenced this pull request Oct 12, 2021
@weierophinney
fix: conflict with zend-view, not zend-router
f0e444e 
A bad cut-and-paste led to conflicting with zend-router, instead of zend-view, as a mechanism for replacing the zend-view package, as reported at:

- laminas#81 (comment)

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
@weierophinney weierophinney mentioned this pull request Oct 12, 2021
Merged
@fascinosum
Copy link

fascinosum commented Oct 12, 2021 •
edited
Loading

thank you, @weierophinney

konarshankar07
konarshankar07 commented Oct 13, 2021
View reviewed changes
Copy link
Contributor Author

@konarshankar07 konarshankar07 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@weierophinney
Regarding 6c73cf8
Thanks for finishing the pending work but can you please explain why we need to add the polyfill classes and how you are able to reproduce the ReturnTypeWillChange error?
I'm new to the laminas so curious about this commit

@froschdesign
Copy link
Member

@konarshankar07

…why we need to add the polyfill classes…

Components like laminas-filter and laminas-navigation do not yet support PHP 8.1, so the polyfills were created. Otherwise we have to wait for 3 other components and the related updates.

…and how you are able to reproduce the ReturnTypeWillChange error?

Were you able to try the hint from the comments in laminas-crypt?

@konarshankar07
Copy link
Contributor Author

konarshankar07 commented Oct 13, 2021 •
edited
Loading

@froschdesign

Were you able to try the hint from the comments in laminas-crypt?

Yes, I found that the test runs are executing with the php8.1.0RC4 and in my local I'm running the test using php8.1.0RC2 version

@froschdesign
Copy link
Member

@konarshankar07
Right: https://github.com/laminas/laminas-view/runs/3873270121?check_suite_focus=true#step:3:18

Were you able to reproduce the errors in your local test environment?

@konarshankar07
Copy link
Contributor Author

@froschdesign

Were you able to reproduce the errors in your local test environment?

Nope, I'm not seeing any error in my local with the php8.1.0RC2

@weierophinney
Copy link
Member

@konarshankar07

The trick is adding the convertDeprecationsToExceptions="true" configuration to the phpunit.xml.dist file.

Prior to PHPUnit 9.5.10, this was the default, which meant that any deprecations from the engine or other code would trigger an exception, which, unless you had an expectDeprecation() directive in the test, would cause it to error, and thus cause CI to fail.

With 9.5.10, PHPUnit decided to toggle that off.

We actually want it ON, because it allows us to identify deprecations we are relying on in our libraries early, and adapt the code to work for future versions. That was the change I made that caused tests to fail, and then required changes so that we could complete the patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Enhancement hacktoberfest-accepted Issues/Pull-Requests which can be fixed during Hacktoberfest: https://hacktoberfest.digitalocean.com
Projects
None yet
Milestone
2.13.0
Development

Successfully merging this pull request may close these issues.
4 participants
@konarshankar07 @weierophinney @froschdesign @fascinosum