Skip to content
This repository has been archived by the owner on Jul 22, 2021. It is now read-only.

Feat: symfony container #1

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

bpolaszek
Copy link
Collaborator

This 1st PR provides 2 helpers:

  • app() giving access to the Kernel
  • container() giving access to the container. When APP_ENV is test, Symfony exposes a special container where all services are public. If it exists, it is picked by default.

@bpolaszek
Copy link
Collaborator Author

Hello @nunomaduro,

PHPStan barks because:

  • Function Pest\Symfony\container() has parameter $app with a nullable type declaration.
  • Function Pest\Symfony\container() has parameter $app with null as default value.

What's actually wrong with this?

  • Function Pest\Symfony\container() should return Symfony\Component\DependencyInjection\ContainerInterface but returns object|null.

That's the point of the service locator. Can I safely add a phpstan-ignore directive on that one?

@bpolaszek bpolaszek force-pushed the feat-symfony-container branch from 77bb5ad to d40fb16 Compare September 26, 2020 06:30
@bpolaszek bpolaszek force-pushed the feat-symfony-container branch from d40fb16 to 66fb262 Compare September 26, 2020 06:44
Copy link
Collaborator

@Jibbarth Jibbarth left a comment

Choose a reason for hiding this comment

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

Hi @bpolaszek ! Great work 👏

For the nullable parameter error in phpstan, the rule come from https://github.com/ergebnis/phpstan-rules#functionsnoparameterwithnullabletypedeclarationrule

In this case, I think you can ignore them in phpstan.neon or via @phpstan-ignore-next-line annotation.

src/App.php Show resolved Hide resolved
src/App.php Outdated Show resolved Hide resolved
@bpolaszek bpolaszek force-pushed the feat-symfony-container branch from f165414 to ecaa133 Compare September 27, 2020 13:41
@bpolaszek bpolaszek force-pushed the feat-symfony-container branch from ecaa133 to 1feb0a7 Compare September 27, 2020 13:46
src/Plugin.php Outdated Show resolved Hide resolved
Copy link
Member

@nunomaduro nunomaduro left a comment

Choose a reason for hiding this comment

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

Everything sounds good for me, just those small comments.

composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
@bpolaszek
Copy link
Collaborator Author

CI is green! 🎉

Copy link
Collaborator

@Jibbarth Jibbarth left a comment

Choose a reason for hiding this comment

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

Wonderful 🎉

@nunomaduro
Copy link
Member

This is good to merge? No updates needed on the docs?

@bpolaszek
Copy link
Collaborator Author

This is good to merge? No updates needed on the docs?

Maybe some doc entries would be cool indeed. I'll write them ASAP, and I think they should be part of the PR (unless you prefer I do a separate one).

Sorry to produce code in drops, I do that in my spare time among other things (as we all do in OSS, but it's always frustrating that weeks pass and nothing lands...).

@nunomaduro
Copy link
Member

Docs should go here: https://github.com/pestphp/docs.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants