Skip to content
This repository has been archived by the owner on Nov 20, 2019. It is now read-only.

feat(package): remove dependency on ext-intl #8

Merged

Conversation

alexander-schranz
Copy link
Contributor

fixes #7

@alexander-schranz
Copy link
Contributor Author

alexander-schranz commented Nov 15, 2016

@layershifter dont know why the scrutinizer-ci errored on composer

@layershifter
Copy link
Owner

layershifter commented Nov 15, 2016

@alexander-schranz Thanks for PR!

I'm glad to use Symfony's polyfill, but it doesn't provide idn_to_utf8 function. Test build's result in this PR isn't correct because intl is enabled by default in envirioment and it cannot be removed 🙁


According to PHP docs idn_to_utf8 uses IDNA 2003 for transforms and we can try to use true/php-punycode, but it will require ext-mbstring 😢

So, here is TODO list for removing ext-intl dependency:

Looks little complicated 😄 I'll start to work on it today.

@alexander-schranz
Copy link
Contributor Author

alexander-schranz commented Nov 15, 2016

@layershifter great, thank you! mbstring can also be used from symfony: https://github.com/symfony/polyfill-mbstring

@Marlinc
Copy link

Marlinc commented Nov 16, 2016

We've made a new release of https://github.com/true/php-punycode in which https://github.com/symfony/polyfill-mbstring can be used

@alexander-schranz
Copy link
Contributor Author

thank you @Marlinc

@layershifter layershifter changed the title Added symfony intl polyfill feat(package): remove dependency on ext-intl Nov 16, 2016
@layershifter
Copy link
Owner

layershifter commented Nov 16, 2016

Two steps completed, let's complete last 😄

I think it's not good idea to send idn_to_utf8 and idn_to_ascii functions into global. Our implementation isn't real polyfill and it's can cause conflicts with other packages.

Let's make a small refactoring there, it will give following advantages:

  • function_exists will called once;
  • Punycode object will be created once.

Extract.php

    /**
     * @var int Value of extraction options.
     */
    private $extractionMode;
+   /**
+    * @var IDN Object of TLDExtract\IDN class.
+    */
+   private $idn;
    /**
     * @var string Name of class that will store results of parsing.
     */
    private $resultClassName;
        public function __construct($databaseFile = null, $resultClassName = null, $extractionMode = null)
    {
+       $this->idn = new IDN();
        $this->suffixStore = new Store($databaseFile);
        if ($isPunycoded) {
-             $hostname = idn_to_utf8($hostname);
+            $hostname = $this->idn->toUTF8($hostname);
        }
-        return $isPunycoded ? idn_to_ascii($suffix) : $suffix;
+        return $isPunycoded ? $this->idn->toASCII($hostname) : $suffix;

IDN.php

<?php

namespace LayerShifter\TLDExtract;

use TrueBV\Punycode;

/**
 * Class that transforms IDN domains, if `intl` extension present uses it.
 */
class IDN
{

    /**
     * @var Punycode Object of TrueBV\Punycode class.
     */
    private $transformer;

    /**
     * Constructor.
     */
    public function __construct()
    {
        if (!function_exists('\idn_to_utf8')) {
            $this->transformer = new Punycode();
        }
    }

    /**
     * Converts domain name from Unicode to IDNA ASCII.
     *
     * @param string $domain Domain to convert in IDNA ASCII-compatible format.
     *
     * @return string
     */
    public function toASCII($domain)
    {
        if ($this->transformer) {
            return $this->transformer->encode($domain);
        }

        return idn_to_ascii($domain);
    }

    /**
     * Converts domain name from IDNA ASCII to Unicode.
     *
     * @param string $domain Domain to convert in Unicode format.
     *
     * @return string
     */
    public function toUTF8($domain)
    {
        if ($this->transformer) {
            return $this->transformer->decode($domain);
        }

        return idn_to_utf8($domain);
    }
}

IDNTest.php

<?php

namespace LayerShifter\TLDExtract\Tests;

use LayerShifter\TLDExtract\IDN;
use TrueBV\Punycode;

/**
 * Tests for IDN class.
 */
class IDNTest extends \PHPUnit_Framework_TestCase
{

    /**
     * @var IDN Object for tests
     */
    private $idn;

    /**
     * Method that setups test's environment.
     *
     * @return void
     */
    protected function setUp()
    {
        $this->idn = new IDN();
    }

    /**
     * Tests constructor(), ensures that transformer isn't loaded when `intl` extension present.
     *
     * @void
     */
    public function testConstructor()
    {
        if (function_exists('\idn_to_utf8')) {
            $this->assertAttributeInternalType('null', 'transformer', $this->idn);

            return;
        }

        $this->assertAttributeInstanceOf(Punycode::class, 'transformer', $this->idn);
    }

    /**
     * Tests toASCII() method.
     *
     * @return void
     */
    public function testToASCII()
    {
        $this->assertEquals('xn--tst-qla.de', $this->idn->toASCII('täst.de'));
    }

    /**
     * Tests toUTF8() method.
     *
     * @return void
     */
    public function testToUTF8()
    {
        $this->assertEquals('täst.de', $this->idn->toUTF8('xn--tst-qla.de'));
    }
}

@layershifter
Copy link
Owner

@alexander-schranz Thanks much, I'll merge it and prepare release 👍

@layershifter layershifter merged commit 22d0e94 into layershifter:master Nov 17, 2016
@alexander-schranz
Copy link
Contributor Author

@layershifter that was to early 🙈

@layershifter
Copy link
Owner

Yep, I saw that CI tests are failed, will fix it before release 🐵

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get rid of the intl extension requirement
3 participants