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

[4.x] Simplifying queuing internal middleware #2765

Merged
merged 3 commits into from Jul 29, 2019

Conversation

@adriansuter
Copy link
Contributor

commented Jul 29, 2019

This PR addresses #2762.

Not sure, if the test cases test everything that needs to be tested.

@adriansuter adriansuter added this to the 4.0.0 milestone Jul 29, 2019

@adriansuter adriansuter added the Slim 4 label Jul 29, 2019

@coveralls

This comment has been minimized.

Copy link

commented Jul 29, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 7246eff on adriansuter:patch-middleware-helpers into 04be469 on slimphp:4.x.

@adriansuter adriansuter requested a review from l0gicgate Jul 29, 2019

@l0gicgate

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

Looks good to me @adriansuter, great work! As for the tests, the middleware test suite was already covering most of this anyway but it's good you went above and beyond with it! We just need to update the docs to reflect those changes. I'm going to merge.

@l0gicgate l0gicgate merged commit 9e7b1d2 into slimphp:4.x Jul 29, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
public function addErrorMiddleware(
bool $displayErrorDetails,
bool $logErrors,
bool $logErrorDetails

This comment has been minimized.

Copy link
@shadowhand

shadowhand Jul 29, 2019

Contributor

In the interest of simplification, should these default to true?

This comment has been minimized.

Copy link
@l0gicgate

l0gicgate Jul 29, 2019

Contributor

I think we need to keep the signature the same as the middleware implementation. Wasn't there someone claiming that true defaults were the root of all evil here not long ago? @akrabat 😂

This comment has been minimized.

Copy link
@adriansuter

adriansuter Jul 30, 2019

Author Contributor

Also the defaults would depend on the environment. In development a user might want to have all set to true, but in production one should not display error details (security issue). So I think, we should keep it as is. So the user has to think about these flags.

This comment has been minimized.

Copy link
@akrabat

akrabat Jul 30, 2019

Member

Defaults are difficult :)

This comment has been minimized.

Copy link
@akrabat

akrabat Jul 30, 2019

Member

Do we actually need logErrorDetails? i.e. what's the use-case for logging errors, but not the details?

This comment has been minimized.

Copy link
@adriansuter

adriansuter Jul 30, 2019

Author Contributor

@akrabat Probably disk memory limitation concerns?

@adriansuter adriansuter deleted the adriansuter:patch-middleware-helpers branch Jul 30, 2019

@l0gicgate l0gicgate referenced this pull request Aug 1, 2019

Merged

Slim 4 Release #2769

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.