Skip to content

Conversation

XWB
Copy link

@XWB XWB commented Jan 2, 2020

Fixes #158

The change introduced in 366a549 breaks our application.

@dbu
Copy link
Contributor

dbu commented Jan 2, 2020

i don't think this is the correct fix. @Nyholm made a precondition based on an interface. if the scenario does have that interface available but is still not able to instantiate, we need to add additional criteria for instantiation.

i guess the problem is that checking for the interface can not tell if any implementation is available. but if there is a whole bunch of possible implementations, i dunno how we can determine if there is one of them present... @Nyholm do you have an idea how to handle it? could we tentatively call some factory to see if it finds a solution?

@Nyholm
Copy link
Member

Nyholm commented Jan 2, 2020

I love this library but the past few months it feels like we've done too many patch fixes..
A few weeks ago I introduced some additional testing to avoid this kind of issue.

@XWB Can you have a look here: https://github.com/php-http/discovery/blob/1.7.3/.travis.yml#L59-L67 and provide your own test that represent your application to show what is broken?

@XWB
Copy link
Author

XWB commented Jan 2, 2020

I''l try to make a test. In the meantime, here's the stack trace:

 at Http\Discovery\Exception\DiscoveryFailedException::create(array(object(PuliUnavailableException), object(NoCandidateFoundException), object(NoCandidateFoundException)))
     (vendor/php-http/discovery/src/ClassDiscovery.php:79)
  at Http\Discovery\ClassDiscovery::findOneByType('Psr\\Http\\Message\\ResponseFactoryInterface')
     (vendor/php-http/discovery/src/Psr17FactoryDiscovery.php:53)
  at Http\Discovery\Psr17FactoryDiscovery::findResponseFactory()
     (vendor/symfony/http-client/HttplugClient.php:79)
  at Symfony\Component\HttpClient\HttplugClient->__construct()
     (vendor/php-http/discovery/src/ClassDiscovery.php:203)
  at Http\Discovery\ClassDiscovery::instantiateClass('Symfony\\Component\\HttpClient\\HttplugClient')
     (vendor/php-http/discovery/src/HttpAsyncClientDiscovery.php:40)
  at Http\Discovery\HttpAsyncClientDiscovery::find()
     (vendor/friendsofsymfony/http-cache/src/ProxyClient/HttpDispatcher.php:99)

As you can see, it's trying to initialize Psr\\Http\\Message\\ResponseFactoryInterface which doesn't work.

@Nyholm
Copy link
Member

Nyholm commented Jan 2, 2020

That is not what I see. =)

I see that you are finding the Symfony HTTPlug client. When instantiating HttplugClient we need a Psr\\Http\\Message\\ResponseFactoryInterface so we are trying to find one, but fails.

I thought this test would cover your case:
https://github.com/php-http/discovery/blob/1.7.3/.travis.yml#L67

Are you using version 1.7.3?

@XWB
Copy link
Author

XWB commented Jan 2, 2020

I do have 1.7.3 installed:

$ composer show | grep discovery

php-http/discovery                  1.7.3      Finds installed HTTPlug implementations and PSR-7 message factories

And everything works fine when I revert 366a549 so the interface_exists must break something?

@Nyholm
Copy link
Member

Nyholm commented Jan 2, 2020

Please add a test similar to the one I linked.
It is only one row where you specify your dependencies. It will be interesting to see.

@XWB
Copy link
Author

XWB commented Jan 2, 2020

@Nyholm Done. I added two tests with friendsofsymfony/http-cache-bundle and you can see the error in Travis:

PHP Fatal error:  Uncaught Http\Discovery\Exception\DiscoveryFailedException: Could not find resource using any discovery strategy. Find more information at http://docs.php-http.org/en/latest/discovery.html#common-errors
 - Puli Factory is not available
 - No valid candidate found using strategy "Http\Discovery\Strategy\CommonClassesStrategy". We tested the following candidates: .
 - No valid candidate found using strategy "Http\Discovery\Strategy\CommonPsr17ClassesStrategy". We tested the following candidates: Nyholm\Psr7\Factory\Psr17Factory, Zend\Diactoros\ResponseFactory, Http\Factory\Diactoros\ResponseFactory, Http\Factory\Guzzle\ResponseFactory, Http\Factory\Slim\ResponseFactory.
 in /home/travis/build/php-http/discovery/build/504b61b/vendor/php-http/discovery/src/Exception/DiscoveryFailedException.php:41
Stack trace:
#0 /home/travis/build/php-http/discovery/build/504b61b/vendor/php-http/discovery/src/ClassDiscovery.php(79): Http\Discovery\Exception\DiscoveryFailedException::create(Array)
#1 /home/travis/build/php-http/ in /home/travis/build/php-http/discovery/build/504b61b/vendor/php-http/discovery/src/ClassDiscovery.php on line 210

@onEXHovia
Copy link

✋ I don’t know if it will be useful. In project we use packages with versions:

"friendsofsymfony/http-cache-bundle": "^2.8"
"php-http/guzzle6-adapter": "^1.1"
"php-http/httplug-bundle": "^1.17"

php-http/discovery have version 1.7.3, current set packages work correct, but after install symfony/http-client@v4.4.2 i get the same exception which mentioned in the comment.

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

I pushed a small change to your branch.
The test will pass now.

I did a similar fix in: https://github.com/php-http/discovery/pull/157/files#diff-f6115264dc0232837d2b0b916ccb41b8R77

But I forgot the async one.

# Test that we find a client with Symfony and Guzzle
- install_test will-find "Http\Discovery\HttpClientDiscovery::find();" "friendsofsymfony/http-cache-bundle:2.4.* symfony/http-client:4.* php-http/guzzle6-adapter"
# Test that we find an async client with Symfony and Guzzle
- install_test will-find "Http\Discovery\HttpAsyncClientDiscovery::find();" "friendsofsymfony/http-cache-bundle:2.4.* symfony/http-client:4.* php-http/guzzle6-adapter"
Copy link
Member

Choose a reason for hiding this comment

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

You can replace friendsofsymfony/http-cache-bundle:2.4.* with php-http/client-common:2.* php-http/message:1.8*

It is the same effect.

@Nyholm Nyholm changed the title Fixes ClassDiscovery::safeClassExists Adding tests for Symfony Httplug Async Jan 2, 2020
@Nyholm
Copy link
Member

Nyholm commented Jan 2, 2020

I also added #161. Both will fix this issue. Which one do we prefer?

@XWB
Copy link
Author

XWB commented Jan 2, 2020

@Nyholm I manually applied your fix in our application and it still fails :(

image

image

@XWB
Copy link
Author

XWB commented Jan 2, 2020

But #161 fixes the issue! :)

@Nyholm
Copy link
Member

Nyholm commented Jan 3, 2020

@Nyholm Nyholm closed this Jan 3, 2020
@XWB XWB deleted the class_exists branch January 3, 2020 11:39
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.

ClassDiscovery::safeClassExists broken
4 participants