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

Drop compatibility with callable response factories on MethodNotAllowedMiddleware and ImplicitOptionsMiddleware #69

Merged
merged 4 commits into from
Jul 18, 2024

Conversation

gsteel
Copy link
Member

@gsteel gsteel commented Jul 18, 2024

Q A
BC Break yes
QA yes

Description

  • Remove compat with callable response factories - only accept ResponseFactoryInterface
  • Removes the now unused response factory trait
  • Removes the unused CallableResponseFactoryDecorator

@gsteel gsteel added this to the 4.0.0 milestone Jul 18, 2024
@gsteel gsteel changed the title Drop compatibility with callable response factories on MethodNotAllowedMiddleware Drop compatibility with callable response factories on MethodNotAllowedMiddleware and ImplicitOptionsMiddleware Jul 18, 2024
Comment on lines +25 to +30
if (! $container->has(ResponseFactoryInterface::class)) {
throw MissingDependencyException::dependencyForService(
ResponseFactoryInterface::class,
ImplicitOptionsMiddleware::class,
);
}
Copy link
Member

Choose a reason for hiding this comment

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

Do we really want to have this kind of exception handling in each factory?

Feels simpler to improve laminas/laminas-servicemanager to better inspect traces, perhaps?

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually think that the traces are already easy enough to figure out, without this kind of specific exception, but I was just following convention here and didn't think it was worth breaking BC with documented @throws

Copy link
Member

Choose a reason for hiding this comment

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

Works for me, it's just something we should perhaps improve in upstream, by rendering better traces in exceptions in a future iteration.

We can decide to drop them overall from this component in a later patch: the redundant ->has() calls all add up quickly, if every service needs to do them

@gsteel gsteel self-assigned this Jul 18, 2024
gsteel added 4 commits July 18, 2024 17:03
…wedMiddleware`

Signed-off-by: George Steel <george@net-glue.co.uk>
…are`

Signed-off-by: George Steel <george@net-glue.co.uk>
Signed-off-by: George Steel <george@net-glue.co.uk>
Signed-off-by: George Steel <george@net-glue.co.uk>
@gsteel gsteel force-pushed the v4/method-not-allowed-response-factory branch from 82ab78e to 567cc25 Compare July 18, 2024 16:04
@gsteel gsteel merged commit 7207f49 into mezzio:4.0.x Jul 18, 2024
11 checks passed
@gsteel gsteel deleted the v4/method-not-allowed-response-factory branch July 18, 2024 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BC Break Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants