Skip to content

Bugfixes #71

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

Merged
merged 4 commits into from
Jul 5, 2016
Merged

Bugfixes #71

merged 4 commits into from
Jul 5, 2016

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Jul 3, 2016

Sorry for this PR. I've made some mistakes in previous PRs.

Fix 1:
We should see if the candidate got a condition when we fetch it from the cache.

Fix 2:
Improved the doc blocs

Fix 3:
We should not require an object of type DiscoveryStrategy on appendStratey and prependStrategy. We should require a string FQCN.

@@ -109,7 +110,7 @@ private static function storeInCache($type, $class)
/**
* Set new strategies and clear the cache.
*
* @param DiscoveryStrategy[] $strategies
* @param array $strategies string array of FQN to a DiscoveryStrategy
Copy link
Member

Choose a reason for hiding this comment

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

FQCN you mean?

Copy link
Contributor

@dbu dbu Jul 4, 2016

Choose a reason for hiding this comment

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

i'd prefer to write it out anyways :-)

@param string[] $strategies Fully qualified class name to a DiscoveryStrategy

*/
public static function prependStrategy(DiscoveryStrategy $strategy)
public static function prependStrategy($strategy)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a full functional test with a prepend strategy that returns a mock?

@dbu
Copy link
Contributor

dbu commented Jul 5, 2016

LGTM!

@sagikazarmark sagikazarmark merged commit 410465b into php-http:master Jul 5, 2016
@Nyholm
Copy link
Member Author

Nyholm commented Jul 5, 2016

Thank you for merging.

@Nyholm Nyholm deleted the strategy-tpo branch July 5, 2016 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants