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

Allow PHP 8.1 #40

Merged
merged 1 commit into from
Jun 3, 2022
Merged

Allow PHP 8.1 #40

merged 1 commit into from
Jun 3, 2022

Conversation

snapshotpl
Copy link
Member

@snapshotpl snapshotpl commented May 11, 2022 •
edited
Loading

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

Description

Allow install on PHP 8.1

Needs:

@snapshotpl snapshotpl added the Enhancement New feature or request label May 11, 2022
@snapshotpl snapshotpl added this to the 1.8.0 milestone May 11, 2022
Ocramius
Ocramius requested changes May 11, 2022
View reviewed changes
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.

Welp, - Root composer.json requires laminas/laminas-coding-standard ~2.2.0 -> satisfiable by laminas/laminas-coding-standard[2.2.0, 2.2.1].

Guess laminas/laminas-coding-standard:~2.3 is needed here.

Ocramius
Ocramius requested changes May 11, 2022
View reviewed changes
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. 

 1) LaminasTest\ApiTools\OAuth2\Controller\AuthControllerTest::testResource
Laminas\View\Exception\DomainException: Laminas\View\Renderer\PhpRenderer::render: received View Model argument, but template is empty

Meanwhile, PHP 7.3 can go away :)

@snapshotpl
Copy link
Member Author

Fix requested bshaffer/oauth2-server-php#1021

Ocramius
Ocramius requested changes May 11, 2022
View reviewed changes
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.

For the CS issues, could we perhaps switch to psr-11, if servicemanager is forced to be at the last version? 🤔

@snapshotpl snapshotpl force-pushed the php-8.1 branch 2 times, most recently from a82b120 to d77a464 Compare May 13, 2022 08:50
@snapshotpl
Copy link
Member Author

CS issue fixed, so now waiting for bshaffer/oauth2-server-php#1021

@snapshotpl snapshotpl force-pushed the php-8.1 branch 3 times, most recently from b5edf3b to 1dddb58 Compare June 1, 2022 09:36
@snapshotpl
Copy link
Member Author

After squash looks like GA stuck, but before all passed https://github.com/laminas-api-tools/api-tools-oauth2/actions/runs/2420613249 so just waiting for new release oauth2 by @bshaffer

Ocramius
Ocramius requested changes Jun 1, 2022
View reviewed changes
composer.json Outdated Show resolved Hide resolved
src/Factory/PdoAdapterFactory.php Outdated Show resolved Hide resolved
@snapshotpl snapshotpl force-pushed the php-8.1 branch 2 times, most recently from 31410f6 to 2cb83f3 Compare June 2, 2022 14:30
Ocramius
Ocramius requested changes Jun 2, 2022
View reviewed changes
src/Factory/AuthControllerFactory.php
* @param null|string $requestedName
* @return AuthController
*/
public function createService(ServiceLocatorInterface $controllers, $name = null, $requestedName = null)
Copy link
Member

Choose a reason for hiding this comment

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

@snapshotpl if we keep the BC breaks in, we need a major: can we keep the dead public methods for a while, and mark them @deprecated instead?

src/Factory/AuthControllerFactory.php Outdated

class AuthControllerFactory implements FactoryInterface
class AuthControllerFactory
Copy link
Member

Choose a reason for hiding this comment

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

Can we somehow keep FactoryInterface to avoid a BC break? 🤔

@bshaffer
Copy link

bshaffer commented Jun 2, 2022

CS issue fixed, so now waiting for bshaffer/oauth2-server-php#1021

Fixed in https://github.com/bshaffer/oauth2-server-php/releases/tag/v1.12.1

@snapshotpl
Copy link
Member Author

Thanks @bshaffer! All tests are green now!

@snapshotpl
Copy link
Member Author

Not sure why php-cs enforced interop\container lowercase 🤔 . But still works

@snapshotpl snapshotpl requested a review from Ocramius June 2, 2022 16:03
@Ocramius
Copy link
Member

Ocramius commented Jun 3, 2022

Not sure why php-cs enforced interop\container lowercase thinking . But still works

It will break autoloading (depending on autoloading order), so I think we need to either ignore or skip the rule for now 🤔

@snapshotpl snapshotpl force-pushed the php-8.1 branch 2 times, most recently from 48d8579 to 99673fe Compare June 3, 2022 08:47
Ocramius
Ocramius approved these changes Jun 3, 2022
View reviewed changes
src/Factory/PdoAdapterFactory.php Outdated Show resolved Hide resolved
@snapshotpl
Allow php 8.1
2af4fe4 
Signed-off-by: Witold Wasiczko <witold@wasiczko.pl>
@Ocramius
Copy link
Member

Ocramius commented Jun 3, 2022

Excellent, also thanks for squashing!

@Ocramius Ocramius self-assigned this Jun 3, 2022
Ocramius
Ocramius approved these changes Jun 3, 2022
View reviewed changes
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.

Thanks @snapshotpl!

@Ocramius Ocramius merged commit 3eac9d1 into laminas-api-tools:1.8.x Jun 3, 2022
@Ocramius Ocramius changed the title Allow php 8.1 Allow PHP 8.1 Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ocramius Ocramius Ocramius approved these changes

Assignees

@Ocramius Ocramius

Labels
Enhancement New feature or request
Projects
None yet
Milestone
1.8.0
Development

Successfully merging this pull request may close these issues.
3 participants
@snapshotpl @bshaffer @Ocramius