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

[6.5] Install symfony's intl-idn polyfill #2550

Merged
merged 4 commits into from Apr 15, 2020
Merged

[6.5] Install symfony's intl-idn polyfill #2550

merged 4 commits into from Apr 15, 2020

Conversation

GrahamCampbell
Copy link
Member

Composer will now, instead, suggest ext-intl for better performance, instead of suggesting ext-intl and having Guzzle crash at runtime with an error people won't understand.

@GrahamCampbell
Copy link
Member Author

GrahamCampbell commented Jan 12, 2020

NB:

    /**
     * @expectedException \GuzzleHttp\Exception\InvalidArgumentException
     * @expectedExceptionMessage IDN conversion failed (errors: IDNA_ERROR_LEADING_HYPHEN)
     */
    public function testExceptionOnInvalidIdn()
    {
        if (!extension_loaded('intl')) {
            self::markTestSkipped('intl PHP extension is not loaded');
        }
        $mockHandler = new MockHandler([new Response()]);
        $client = new Client(['handler' => $mockHandler]);

        $client->request('GET', 'https://-яндекс.рф/images', ['idn_conversion' => true]);
    }

The following test fails with the polyfill, since they don't seem to detect the leading slash, so I have left the test conditional on the real extension being loaded.

@ChrisThompsonTLDR
Copy link

ChrisThompsonTLDR commented Jan 12, 2020

[cthompson@dev-master example]$ composer up
Loading composer repositories with package information                                                                              Updating dependencies (including require-dev)         Package operations: 8 installs, 0 updates, 0 removals
  - Installing ralouphie/getallheaders (3.0.3): Loading from cache
  - Installing psr/http-message (1.0.1): Loading from cache
  - Installing guzzlehttp/psr7 (1.6.1): Loading from cache
  - Installing guzzlehttp/promises (v1.3.1): Loading from cache
  - Installing symfony/polyfill-php72 (v1.13.1): Loading from cache
  - Installing symfony/polyfill-mbstring (v1.13.1): Loading from cache
  - Installing symfony/polyfill-intl-idn (v1.13.1): Loading from cache
  - Installing guzzlehttp/guzzle (dev-patch-2 7180cd1): Cloning 7180cd1936 from cache
guzzlehttp/psr7 suggests installing zendframework/zend-httphandlerrunner (Emit PSR-7 responses)
symfony/polyfill-intl-idn suggests installing ext-intl (For best performance)
guzzlehttp/guzzle suggests installing psr/log (Required for using the Log middleware)
Writing lock file
Generating autoload files
<?php

require __DIR__ . '/vendor/autoload.php';

$client = new GuzzleHttp\Client();
$response = $client->request('get', 'www.google.com', [
    \GuzzleHttp\RequestOptions::IDN_CONVERSION => true,
]);

var_dump($response->getStatusCode()); // outputs 200
[cthompson@dev-master example]$ php guzzle.php
PHP Fatal error:  Uncaught Error: Call to undefined function GuzzleHttp\_idn_uri_convert() in /var/www/sites/www.example.com/example/vendor/guzzlehttp/guzzle/src/Client.php:220
Stack trace:
#0 /var/www/sites/www.example.com/example/vendor/guzzlehttp/guzzle/src/Client.php(155): GuzzleHttp\Client->buildUri(Object(GuzzleHttp\Psr7\Uri), Array)
#1 /var/www/sites/www.example.com/example/vendor/guzzlehttp/guzzle/src/Client.php(183): GuzzleHttp\Client->requestAsync('get', 'www.google.com', Array)
#2 /var/www/sites/www.example.com/example/guzzle.php(7): GuzzleHttp\Client->request('get', 'www.google.com', Array)
#3 {main}
  thrown in /var/www/sites/www.example.com/example/vendor/guzzlehttp/guzzle/src/Client.php on line 220

@GrahamCampbell
Copy link
Member Author

@ChrisThompsonTLDR You'll need to combine both my PRs, since patch-2 only contains support for intl without the intl extension, but not my first fix.

@GrahamCampbell
Copy link
Member Author

I've pushed a branch chris that contains both fixes, you can test with.

@ChrisThompsonTLDR
Copy link

@GrahamCampbell your branch chris works for me in both of the following scenarios.

<?php

require __DIR__ . '/vendor/autoload.php';

$client = new GuzzleHttp\Client();
$response = $client->request('get', 'www.google.com', [
    \GuzzleHttp\RequestOptions::IDN_CONVERSION => true,
]);

var_dump($response->getStatusCode()); // outputs 200

results in

[cthompson@dev-master example]$ php guzzle.php
int(200)

and then

<?php

require __DIR__ . '/vendor/autoload.php';

$client = new GuzzleHttp\Client();
$response = $client->request('get', 'www.google.com');

var_dump($response->getStatusCode()); // outputs 200

results in

[cthompson@dev-master example]$ php guzzle.php
int(200)

composer up looks like

[cthompson@dev-master example]$ composer up
Loading composer repositories with package information                                                                              Updating dependencies (including require-dev)         Package operations: 8 installs, 0 updates, 0 removals
  - Installing ralouphie/getallheaders (3.0.3): Loading from cache
  - Installing psr/http-message (1.0.1): Loading from cache
  - Installing guzzlehttp/psr7 (1.6.1): Loading from cache
  - Installing guzzlehttp/promises (v1.3.1): Loading from cache
  - Installing symfony/polyfill-php72 (v1.13.1): Loading from cache
  - Installing symfony/polyfill-mbstring (v1.13.1): Loading from cache
  - Installing symfony/polyfill-intl-idn (v1.13.1): Loading from cache
  - Installing guzzlehttp/guzzle (dev-chris dd6f31c): Cloning dd6f31cb36 from cache
guzzlehttp/psr7 suggests installing zendframework/zend-httphandlerrunner (Emit PSR-7 responses)
symfony/polyfill-intl-idn suggests installing ext-intl (For best performance)
guzzlehttp/guzzle suggests installing psr/log (Required for using the Log middleware)
Writing lock file
Generating autoload files

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.

Thank you

@Nyholm Nyholm merged commit 3325c9d into guzzle:6.5 Apr 15, 2020
@GrahamCampbell GrahamCampbell deleted the patch-2 branch April 15, 2020 18:28
Nyholm added a commit that referenced this pull request Apr 18, 2020
* Travis improvements

* Don't use internal functions (#2548)

* [6.5] Install symfony's intl-idn polyfill (#2550)

* Install symfony's intl-idn polyfill

* idn conversion always available

* remove "skipped because of intl"

* PHPStan CI fix

Co-authored-by: Nyholm <tobias.nyholm@gmail.com>

* Prepare release of 6.5.3 (#2613)

* Prepare release of 6.5.3

* Typo

* Updated date

* Updated constant

* Fixed composer.json package order

Co-authored-by: Márk Sági-Kazár <sagikazarmark@users.noreply.github.com>
Co-authored-by: Nyholm <tobias.nyholm@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants