Add is_countable under PHP 7.3 #115

carusogabriel · 2018-03-02T17:39:10Z

nicolas-grekas

thank you :)

nicolas-grekas · 2018-03-02T17:45:05Z

src/Php73/bootstrap.php

+
+if (PHP_VERSION_ID < 70300) {
+    if (!function_exists('is_countable')) {
+        function is_countable($s) { return p\Php73::is_countable($s); }


for such a simple implementation, I think we can inline the code a remove the static class (as done on other polyfills AFAIK)

@nicolas-grekas Sure, gonna do this!

nicolas-grekas · 2018-03-02T17:46:04Z

tests/Php73/Php72Test.php

+/**
+ * @author Gabriel Caruso <carusogabriel34@gmail.com>
+ *
+ * @covers Symfony\Polyfill\Php73\Php73::<!public>


should be removed in you accept the previous comment (same below)

keradus · 2018-03-02T17:52:32Z

tests/Php73/Php72Test.php

+        $this->assertTrue(is_countable([1, 2, 3]));
+        $this->assertTrue(is_countable(new \ArrayIterator(['foo', 'bar', 'baz'])));
+        $this->assertTrue(is_countable(new \ArrayIterator()));
+        $this->assertFalse(is_countable(new \stdClass()));


please add a case with generator

@keradus Is this possible with 5.x tests?

even if not, it shall still be tested

stof

the root composer.json needs to be updated to add the replace rule for the new package.

stof · 2018-03-02T18:12:31Z

src/Php73/composer.json

+    "minimum-stability": "dev",
+    "extra": {
+        "branch-alias": {
+            "dev-master": "1.7-dev"


should be 1.8 (and we should bump the version in master before merging this), as this is a new feature and so will be a new minor version, not a new patch version

@stof I'll just wait for the official merge in php-src, I'll do it :)

carusogabriel · 2018-03-10T23:13:04Z

@nicolas-grekas Should be ready to go 😃

keradus

👎

Travis is failing (as you used short-array syntax not yet available for 5.3)
generator test case shall be added, to ensure polyfill works for it properly
tests are suppose to be executed, on different environments, with polyfill (on PHP versions that don't have the feature yet), and without it (using real feature). here, we always run test with a polyfill, never on PHP 7.3, as 7.3 is not part of Travis matrix

carusogabriel · 2018-03-12T10:52:08Z

tests/Php73/Php73Test.php

+        for ($i = 1; $i <= 10; ++$i) {
+            yield $i;
+        }
+    }


@keradus Is there a way to add generators tests without breaking the build?

the yield code would need to be extracted to dedicated file and loaded conditionally

@keradus Done, thanks.

carusogabriel · 2018-03-12T10:52:36Z

tests/Php73/Php73Test.php

+     */
+    public function testIsCountable()
+    {
+        $this->assertTrue(is_countable(array(1, 2, '3')));


@keradus Fixed the array syntax, thanks.

keradus · 2018-03-12T13:01:35Z

looks better with every change @carusogabriel !
(btw, if you would not squash, it would be way easier to review the changes, now one need to read whole diff again and again, instead of just patch - don't worry, squash can be done during merging!)

one last thing - could you extend .travis.yml matrix to contain 7.2 and 7.3 ?

carusogabriel · 2018-03-12T13:29:09Z

@keradus

btw, if you would not squash, it would be way easier to review the changes, now one need to read whole diff again and again, instead of just patch - don't worry, squash can be done during merging!

Sure, thanks for the tip, I always amend my commits, but I'll start to commit and then squash them before merging the PR, thanks!

could you extend .travis.yml matrix to contain 7.2 and 7.3 ?

Sure, pushing these changes ASAP.

carusogabriel · 2018-03-12T15:29:56Z

@keradus There's one remain error in PHP 7.3 that I couldn't figure out how to solve. Maybe we should just add 7.2?

keradus · 2018-03-12T18:05:43Z

the feature you are adding has to be tested under 7.3. if not, then we test only "does our polyfill work like we expect", but don't "does our polyfill expectation are same as original implementation".
yet, I agree 7.3 failing is not related to your PR.
sth nice to have for sure, but maybe indeed out of the scope here. nevertheless, adding it (even temporary) to travis matrix has proven that your changes works same as native implementation.

if you ask me, I would be fine with reverting travis changes and do it as separated PR.

keradus · 2018-04-06T11:34:34Z

@stof , your request has been already fulfilled, could you please revisit your review ?

In general, missing thing on Travis is related to Mbstring, thus it's not connected with feature introduced by this PR.
Could we make this one released?

nicolas-grekas · 2018-04-19T08:09:40Z

composer.json

@@ -50,6 +51,7 @@
            "src/Php70/bootstrap.php",
            "src/Php71/bootstrap.php",
            "src/Php72/bootstrap.php",
+            "src/Php73/bootstrap.php",


the branch-alias should be updated to 1.8-dev for all composer.json files of the repo

nicolas-grekas · 2018-04-19T08:09:57Z

src/Php73/LICENSE

@@ -0,0 +1,19 @@
+Copyright (c) 2015-2018 Fabien Potencier


2018 only (not "2015-")

nicolas-grekas · 2018-04-19T08:10:25Z

src/Php73/bootstrap.php

+
+if (PHP_VERSION_ID < 70300) {
+    if (!function_exists('is_countable')) {
+        function is_countable($s) {


the function should be on one single line (to make the test suite work as expected)
$s should be renamed by $var I suppose also.

nicolas-grekas · 2018-04-19T09:47:39Z

Would you mind adding hrtime() also in the PHP 7.3 polyfill (based on microtime())?

  . Added monotonic timer function hrtime([bool get_as_num]). It returns an
    array of the form [seconds, nanoseconds] with the timestamp starting at
    an unspecified point in the past. If the optional argument is passed as
    true, the return value is an integer on 64-bit systems or float on
    32-bit systems, representing the nanoseconds. The timestamp is not
    adjustable and is not related to wall clock or time of day. The timers
    are available under Linux, FreeBSD, Windows, Mac, SunOS, AIX and their
    derivatives. If no required timers are provided by a corresponding
    platform, the function returns false.

keradus · 2018-04-19T10:44:18Z

Could we please add different feature with different PR?
This one is already awaiting for 2.5 months, no need to postpone it even further ;)

nicolas-grekas · 2018-04-25T14:56:31Z

Thank you @carusogabriel.

@nicolas-grekas

This PR was squashed before being merged into the 1.7-dev branch (closes #115). Discussion ---------- Add is_countable under PHP 7.3 As requested by @nicolas-grekas :) Waiting for php/php-src#3026 Commits ------- 803a622 Add is_countable under PHP 7.3

nicolas-grekas · 2018-04-25T15:03:24Z

FYI, I removed the nightly line because it failed for unrelated reason, to be fixed in a next PR. hrtime() welcome now.

Majkl578 · 2018-04-25T17:05:18Z

As noted in the commit here: symfony/polyfill-php73@f713942#r28736995
This implementation is incorrect.

This implementation is not correct. There are objects that are countable, but do not implement Countable, for example SimpleXmlElement: https://3v4l.org/IcOMX

Also see https://github.com/php/php-src/pull/3026/files#r170670721.

…t implement \Countable (pierredup) This PR was merged into the 1.8-dev branch. Discussion ---------- Update is_countable to include countable objects that doesn't implement \Countable Some objects are countable but don't implement `Countable` ([#115#issuecomment-384361414](#115 (comment))) The objects that I could identify that is countable is `ResourceBundle` and `SimpleXmlElement` Commits ------- b53929e Update is_countable to include countable objects that doesn't implement \Countable

…t implement \Countable (pierredup) This PR was merged into the 1.8-dev branch. Discussion ---------- Update is_countable to include countable objects that doesn't implement \Countable Some objects are countable but don't implement `Countable` ([symfony/polyfill#115#issuecomment-384361414](symfony/polyfill#115 (comment))) The objects that I could identify that is countable is `ResourceBundle` and `SimpleXmlElement` Commits ------- b53929e Update is_countable to include countable objects that doesn't implement \Countable

nicolas-grekas reviewed Mar 2, 2018

View reviewed changes

keradus reviewed Mar 2, 2018

View reviewed changes

stof requested changes Mar 2, 2018

View reviewed changes

keradus suggested changes Mar 12, 2018

View reviewed changes

carusogabriel commented Mar 12, 2018

View reviewed changes

keradus approved these changes Mar 12, 2018

View reviewed changes

xabbuh mentioned this pull request Apr 19, 2018

Add PHP 7.3 polyfill #117

Closed

nicolas-grekas reviewed Apr 19, 2018

View reviewed changes

Add is_countable under PHP 7.3

803a622

nicolas-grekas merged commit 803a622 into symfony:master Apr 25, 2018

carusogabriel deleted the is-countable branch April 25, 2018 15:15

pierredup mentioned this pull request Apr 25, 2018

Update is_countable to include countable objects that doesn't implement \Countable #119

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add is_countable under PHP 7.3 #115

Add is_countable under PHP 7.3 #115

carusogabriel commented Mar 2, 2018

nicolas-grekas left a comment

nicolas-grekas Mar 2, 2018

carusogabriel Mar 2, 2018

nicolas-grekas Mar 2, 2018

keradus Mar 2, 2018

carusogabriel Mar 2, 2018

keradus Mar 3, 2018

stof left a comment

stof Mar 2, 2018

carusogabriel Mar 3, 2018

carusogabriel commented Mar 10, 2018

keradus left a comment

carusogabriel Mar 12, 2018

keradus Mar 12, 2018

carusogabriel Mar 12, 2018

carusogabriel Mar 12, 2018

keradus commented Mar 12, 2018

carusogabriel commented Mar 12, 2018 •

edited

Loading

carusogabriel commented Mar 12, 2018

keradus commented Mar 12, 2018

keradus commented Apr 6, 2018

nicolas-grekas Apr 19, 2018

nicolas-grekas Apr 19, 2018

nicolas-grekas Apr 19, 2018

nicolas-grekas commented Apr 19, 2018

keradus commented Apr 19, 2018

nicolas-grekas commented Apr 25, 2018

nicolas-grekas commented Apr 25, 2018

Majkl578 commented Apr 25, 2018 •

edited

Loading

Add is_countable under PHP 7.3 #115

Add is_countable under PHP 7.3 #115

Conversation

carusogabriel commented Mar 2, 2018

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carusogabriel commented Mar 10, 2018

keradus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keradus commented Mar 12, 2018

carusogabriel commented Mar 12, 2018 • edited Loading

carusogabriel commented Mar 12, 2018

keradus commented Mar 12, 2018

keradus commented Apr 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Apr 19, 2018

keradus commented Apr 19, 2018

nicolas-grekas commented Apr 25, 2018

nicolas-grekas commented Apr 25, 2018

Majkl578 commented Apr 25, 2018 • edited Loading

carusogabriel commented Mar 12, 2018 •

edited

Loading

Majkl578 commented Apr 25, 2018 •

edited

Loading