Skip to content
This repository has been archived by the owner. It is now read-only.

ErrorCatcher with RFC7231 accept header process #149 #151

Merged
merged 7 commits into from Nov 14, 2019

Conversation

kamarton
Copy link
Contributor

@kamarton kamarton commented Nov 11, 2019

Q A
Is bugfix?
New feature? ✔️
Breaks BC?
Tests pass? ✔️
Fixed issues #149

src/Helper/HeaderHelper.php Outdated Show resolved Hide resolved
src/Helper/HeaderHelper.php Outdated Show resolved Hide resolved
src/Helper/HeaderHelper.php Outdated Show resolved Hide resolved
src/Helper/HeaderHelper.php Outdated Show resolved Hide resolved
}

/**
* @param $values string|string[]|RequestInterface $values Header value as a comma-separated string
Copy link
Member

@samdark samdark Nov 11, 2019

Choose a reason for hiding this comment

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

It is better to have at least a separate method that works with RequestInterface. Having too many types in a single parameter adds to overall method complexity including its usage.

Copy link
Contributor Author

@kamarton kamarton Nov 14, 2019

Choose a reason for hiding this comment

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

How about using only RequestInterface as input?
I don't know if anyone wants to use this for a non-standard Accept header.

src/Helper/HeaderHelper.php Outdated Show resolved Hide resolved
@@ -99,7 +100,7 @@ private function getRenderer(string $contentType): ?ThrowableRendererInterface

private function getContentType(ServerRequestInterface $request): string
{
$acceptHeaders = preg_split('~\s*,\s*~', $request->getHeaderLine('Accept'), PREG_SPLIT_NO_EMPTY);
$acceptHeaders = HeaderHelper::getSortedAcceptTypes($request);
Copy link
Member

@samdark samdark Nov 11, 2019

Choose a reason for hiding this comment

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

It is the only usage of the helper so far. What do you think about making some of the methods of the helper private?

Copy link
Contributor Author

@kamarton kamarton Nov 12, 2019

Choose a reason for hiding this comment

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

It may be problematic in terms of testability.

Copy link
Member

@samdark samdark Nov 12, 2019

Choose a reason for hiding this comment

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

Why? Tests should reflect usage overall.

Copy link
Contributor Author

@kamarton kamarton Nov 14, 2019

Choose a reason for hiding this comment

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

Why? Tests should reflect usage overall.

This is true. The problem is that e.g. testing the A and B functions that are built into the C function is usually tested with fewer tests than the A, B, and C functions are tested separately. Obviously this is wrong, but usually it will be the result.

@samdark samdark added the status:under development label Nov 11, 2019
kamarton and others added 2 commits Nov 12, 2019
@samdark samdark added status:code review and removed status:under development labels Nov 12, 2019
* @see getValueAndParameters
* @link https://developer.mozilla.org/en-US/docs/Glossary/Quality_values
*/
public static function getByQFactorSortedList($values): array
Copy link
Member

@samdark samdark Nov 12, 2019

Choose a reason for hiding this comment

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

getValuesListSortedByQFactor?

Copy link
Contributor Author

@kamarton kamarton Nov 14, 2019

Choose a reason for hiding this comment

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

getSortedValueAndParameters

No other sorting method found. What I have found is all sorted by q factor, so it's unnecessary to name q factor in the method name.

@samdark samdark added status:under development and removed status:code review labels Nov 12, 2019
@samdark samdark added status:code review and removed status:under development labels Nov 14, 2019
@samdark samdark self-requested a review Nov 14, 2019
@samdark samdark removed the status:code review label Nov 14, 2019
@samdark samdark merged commit d19f66f into yiisoft:master Nov 14, 2019
3 checks passed
@samdark
Copy link
Member

samdark commented Nov 14, 2019

Merged. Thank you!

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.

None yet

2 participants