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

Implement Countable on Objects in PHP7.2 #1378

Merged
merged 1 commit into from Nov 12, 2017

Conversation

Projects
None yet
4 participants
@p365labs
Collaborator

p365labs commented Sep 20, 2017

as reported #1377 in PHP 7.2 it's possibile to use the function count() only on Arrays and on \Countable objects.

This is a WIP PR, there are still things to do and some tests to check; more then this I will merge this PR when PHP 7.2 will be released (now it's a RC2).

@@ -11,7 +11,7 @@
*
* @author Nicolas Ruflin <spam@ruflin.com>
*/
class Response
class Response implements \Countable

This comment has been minimized.

@ruflin

ruflin Sep 20, 2017

Owner

Is this change also needed?

@p365labs p365labs changed the title from [WIP] Implement Countable on Objects in PHP7.2 to Implement Countable on Objects in PHP7.2 Nov 9, 2017

@p365labs

This comment has been minimized.

Collaborator

p365labs commented Nov 9, 2017

@ruflin Now the PR# it's not in WIP anymore, tests now pass also with PHP 7.2-rc (which is now RC5). Today will be released php7.2-rc6 and for the end of the current month PHP7.2 GA will be released. Feel free to merge it or wait till PHP7.2 . GA.

@ruflin

Left a few minor comments. Happy to get this in as soon as the PR is ready independent of which of the versions is out.

@@ -330,7 +330,8 @@ public function toArray()
$this->setQuery(new MatchAll());
}
if (isset($this->_params['post_filter']) && 0 === count($this->_params['post_filter'])) {
$a = $this->_params['post_filter'];

This comment has been minimized.

@ruflin

ruflin Nov 9, 2017

Owner

Nit picking but could we name this variable differently, something more describtive? I'm curious what happens on the next line in case toArray() does not on the varialbe?

@@ -361,4 +361,18 @@ public function getJsonBigintConversion()
{
return $this->_jsonBigintConversion;
}
// /**

This comment has been minimized.

@ruflin

ruflin Nov 9, 2017

Owner

We should remove this code if not needed anymore.

@p365labs

This comment has been minimized.

Collaborator

p365labs commented Nov 10, 2017

@ruflin the pr has been updated :) removed unused code and remove that weird var name :)

@ruflin ruflin merged commit 136e366 into ruflin:master Nov 12, 2017

@p365labs p365labs deleted the p365labs:es6_php72_implement_countable branch Nov 13, 2017

@p365labs p365labs referenced this pull request Nov 15, 2017

Closed

Bug with PHP 7.2 #1377

@XWB

This comment has been minimized.

Contributor

XWB commented Dec 15, 2017

Hi @ruflin

Would it be possible to cherry-pick this commit to the 5.x branch?

@ruflin

This comment has been minimized.

Owner

ruflin commented Dec 19, 2017

@XWB Sound reasonable to me. Do you want to open a backport PR?

XWB pushed a commit to XWB/Elastica that referenced this pull request Dec 19, 2017

@XWB

This comment has been minimized.

Contributor

XWB commented Dec 19, 2017

@ruflin Done #1428

ruflin added a commit that referenced this pull request Dec 20, 2017

@richardfullmer

This comment has been minimized.

richardfullmer commented Feb 23, 2018

The change at https://github.com/ruflin/Elastica/pull/1378/files#diff-e421c6a76338876301c98a5a7e95ab87R333 broke my implementation today since $this->_params['post_filter'] was already an array.

Noticed exception 'Symfony\Component\Debug\Exception\FatalThrowableError' with message 'Call to a member function toArray() on array'

@ruflin

This comment has been minimized.

Owner

ruflin commented Feb 26, 2018

@richardfullmer Is this the same issue as in #1429 ?

@richardfullmer

This comment has been minimized.

richardfullmer commented Feb 26, 2018

Yes, looks the same to me.

jmsche added a commit to leapt/Elastica that referenced this pull request Jun 29, 2018

maximenko pushed a commit to maximenko/Elastica that referenced this pull request Oct 3, 2018

maximenko pushed a commit to maximenko/Elastica that referenced this pull request Oct 3, 2018

ruflin added a commit that referenced this pull request Oct 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment