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

PSR-17 support #43

Closed
wants to merge 3 commits into from
Closed

Conversation

boesing
Copy link
Member

@boesing boesing commented Jul 25, 2021

Q A
BC Break no
New Feature yes

Description

As already done in mezzio/mezzio#75, mezzio/mezzio-router#23 and mezzio/mezzio-problem-details#19 this PR provides support for PSR-17 response factory.

I started removing some prophecy stuff but this is getting too much thus I stopped. Guess I've removed about 50% but I want to focus more on the PSR-17 implementation now. Sorry for not finishing it.

@boesing boesing added the Enhancement New feature or request label Jul 25, 2021
@boesing boesing added this to the 2.2.0 milestone Jul 25, 2021
@boesing boesing added this to In progress in PSR-17 support Jul 25, 2021
@Ocramius Ocramius removed this from the 2.2.0 milestone Dec 4, 2021
@Ocramius
Copy link
Member

Ocramius commented Dec 4, 2021

Removing milestone: I don't have the understanding (right now) to review this patch, so I requested a review from @weierophinney.

Today, I'll focus on PHP 8.1 support.

@tobias-trozowski
Copy link
Contributor

tobias-trozowski commented Dec 4, 2021

is there any greater benefit for making seperate internal methods instead of assertAttributeSame (which uses reflection)?
IMO adding methods (even if they are marked as @internal) solely for the sake of unit testing should really be avoided.

bumping php to 7.4 might also be suitable as 7.3 reached EOL.

@boesing
Copy link
Member Author

boesing commented Dec 4, 2021

@tobias-trozowski
Copy link
Contributor

in case of the MezzioUrlGeneratorFactory the non-failure of the factory is the proof that the instance is constructed accordingly. the optional ?ServerUrlHelper $serverUrlHelper of MezzioUrlGenerator could be validated by calling the generate method as it would return different values based on the null|non-null value of private $serverUrlHelper.

the same principle would apply to $urlHelperServiceName of MezzioUrlGeneratorFactory, by providing different values the result is different.

getMap can be replaced quite easily by just using MetadataMap::get(string $class).

it is a bit a roundabout way to do it but at least it would not litter up the public interface with test-only things.

@boesing
Copy link
Member Author

boesing commented Dec 5, 2021

Since I stopped working on this as of now, I'd be happy to hand this over.
Definitely have time for assistance but no time to work on this myself, so if you do have time and want to work on this, that would be very appreciated. Happy to see your changes in a new PR.
Is that something you are willing to contribute?

@tobias-trozowski tobias-trozowski mentioned this pull request Dec 5, 2021
@tobias-trozowski
Copy link
Contributor

@boesing i created a new PR "PSR-17 support #46" based on your work

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
- removed some prophecy usage
- removed some private attribute assertions

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@weierophinney weierophinney changed the base branch from 2.2.x to 2.3.x December 6, 2021 15:55
Copy link
Contributor

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

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

@boesing I rebased against 2.3.x as we've already released 2.2.0. When I did, I bumped the minimum supported PHP to 7.4... and Psalm then flagged a few additional errors, which I fixed, but which were not really related to this.

I've marked a couple nit-picky changes; but otherwise, this makes sense, and I think it's ready to ship.

return new CallableResponseFactoryDecorator($responseFactory);
}

private function doesConfigurationProvidesDedicatedResponseFactory(ContainerInterface $container): bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private function doesConfigurationProvidesDedicatedResponseFactory(ContainerInterface $container): bool
private function configurationProvidesDedicatedResponseFactory(ContainerInterface $container): bool

Reads with a bit more flow.

Assert::isArrayAccessible($aliases);

if (isset($delegators[ResponseInterface::class]) || isset($aliases[ResponseInterface::class])) {
// Even tho, aliases could point to a different service, we assume that there is a dedicated factory
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Even tho, aliases could point to a different service, we assume that there is a dedicated factory
// Even though aliases could point to a different service, we assume that there is a dedicated factory

@weierophinney
Copy link
Contributor

Closing in favor of #46.

PSR-17 support automation moved this from In progress to Done Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants