-
Notifications
You must be signed in to change notification settings - Fork 61
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
Remove deprecated library #556
Conversation
thanks for the pull request. this will need quite some refactoring. we need to accept the PSR17 factories (in addition to the legacy factories for BC) in places like the (HttpDispatcher)[https://github.com/FriendsOfSymfony/FOSHttpCache/blob/e446b18c2fe80484684c67ebd1848ec726c8a4ad/src/ProxyClient/HttpDispatcher.php#L104] and use the psr17 discovery (but again for BC should fallback to the legacy discovery if not found) the tests need to use the new discovery as well, it seems. for the declared dependencies, i am not sure about removing them in version 2. if somebody instantiates something explicitly, it would break for them once they update. if we allow the new things while still supporting the old, we can tag a last 2.x release and then start 3.x where we actually drop things and completely move to PSR-17. if you have some time to work on this, it would be awesome! |
Hello, |
738dbd1
to
905340f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for looking into this!
@@ -52,7 +52,7 @@ class Cloudflare extends HttpProxyClient implements ClearCapable, PurgeCapable, | |||
public function __construct( | |||
Dispatcher $httpDispatcher, | |||
array $options = [], | |||
RequestFactory $messageFactory = null | |||
RequestFactoryInterface $messageFactory = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to avoid BC breaks, we should remove the type declaration and instead have a phpdoc that does RequestFactory|RequestFactoryInterface
* default one is created | ||
*/ | ||
public function __construct( | ||
array $servers, | ||
$baseUri = '', | ||
HttpAsyncClient $httpClient = null, | ||
UriFactory $uriFactory = null | ||
Psr17FactoryDiscovery $uriFactory = null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and this needs a phpdoc UriFactory|Psr17FactoryDiscovery
and our code needs to handle both cases
src/ProxyClient/HttpDispatcher.php
Outdated
|
||
$this->setServers($servers); | ||
$this->setBaseUri($baseUri); | ||
} | ||
|
||
public function invalidate(RequestInterface $invalidationRequest, $validateHost = true) | ||
{ | ||
var_dump('ttttt'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't forget to remove this when debugging is done
) { | ||
$this->httpDispatcher = $httpDispatcher; | ||
$this->options = $this->configureOptions()->resolve($options); | ||
$this->requestFactory = $messageFactory ?: MessageFactoryDiscovery::find(); | ||
$this->requestFactory = $messageFactory ?: Psr17FactoryDiscovery::findRequestFactory(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should fallback to MessageFactoryDiscovery::find if the Psr17FactoryDiscovery does not find anything.
wrapped up in #566 because we do a new major version, i decided to not provide BC with the old factories. anyways most legacy factories at the same time implement the psr factory as well. |
fix #443 #555