lazy toArray #916

ewgRa · 2015-08-13T20:03:17Z

This is PR for #783, convert object toArray when it is really needed. Comments, tests, review - welcome.

BC:

Now toArray is lazy. This mean that if you set object to Query, Aggregation and so on, and later will change it - changes will be affected at toArray also.
For example such test:

        $query = new Query();
        $aggregation = new \Elastica\Aggregation\Terms('text');

        $query->addAggregation($aggregation);

        $aggregation->setName('another text');

        $anotherQuery = new Query();
        $anotherQuery->addAggregation($aggregation);

        $this->assertEquals($query->toArray(), $anotherQuery->toArray());

with previous code will be not passed. With this PR - it is passed.

if your code work with params somehow (direct access to _params, getParams() methods and so on) - it can be broken.
For example, before:

$script = Elastica\Script::create($script);
return $this->setParams($script->toArray());

Now:

return $this->setParam('script', Elastica\Script::create($script));

Same as some named object:
before:

$this->_aggs[$aggregation->getName()] = $aggregation->toArray();

after:

$this->_aggs[] = $aggregation;

As you can see, aggregation name in toArray also become lazy.

im-denisenko · 2015-08-14T17:05:17Z

lib/Elastica/Nameable.php

+interface Nameable
+{
+    public function getName();
+}


I think that for consistency, interface should include setName as well.

im-denisenko · 2015-08-14T18:15:35Z

Awesome!

I suspect something is broken, but all tests seems to be green, hence lgtm.

I left comments (mostly cosmetic issues) in the code, also could you please:

update changelog
add docblocks to all new methods
run php-cs-fixer
save result of _getBaseName into variable in toArray methods. It's sounds like microoptimization, but here this method was called 6 times, it's too much

ewgRa · 2015-08-14T20:22:42Z

All done, if squash needed or something else, let me know.

Also does I need add some tests for increase code-coverage?

ruflin · 2015-08-16T15:35:01Z

@im-denisenko @ewgRa Looks good to me.
@ewgRa Adding tests is almost always a good thing :-)

lazy toArray

im-denisenko · 2015-08-16T19:26:40Z

@ewgRa Thanks, merged. If you still want add more test for coverage, feel free to open another PR.

ruflin · 2015-08-16T19:32:02Z

Looks like the next Elastica version will be 2.3.0 because of the BC changes. We should get this out of the door as soon as possible and mention the changes. @ewgRa did a good job in the pull request explaining it.

ewgRa · 2015-08-16T21:07:32Z

Ok, thanks!

I will add some tests later.

ruflin · 2015-09-15T14:59:59Z

@ewgRa @im-denisenko There as an issue with the anonymous filters: 5a08b5f I think it is fixed now. The main issue is actually that even though some tests fail on travis, it still reports green. I have to look into this.

ewgRa · 2015-09-15T15:11:07Z

5a08b5f must be not affect of or affected due to this PR, as I understand.

Green travis with fail tests that strange, yes. When you fix it and some test will be broken because of this PR - write it here. As I remember when I work on this PR all tests was passed on my local computer. I will check again when I will be at home.

ruflin · 2015-09-15T16:09:09Z

@ewgRa It looks good now. I just checked the build and this pull request was the first time it was broken. But with the commit above it is fixed.

stof · 2015-09-15T21:18:53Z

lib/Elastica/Aggregation/Filters.php

-            $filterArray[] = $filter->toArray();
+        $filterArray = array();
+
+        if (is_string($name)) {


this looks broken as the default value of the empty string is still a string

@stof Agree. There is something wrong here. Did you spot this because of the discussion or did you run already in some issues with it?

I looked at this PR from the release changelog

@stof Here is the fix that I applied for the current release: 5a08b5f

@ruflin change, that @stof notice I made to sync this check between toArray and addFilter, because they must be same. I think additionally to 5a08b5f there is must be changed addFilter method in same way. What do you think? Or I was wrong when say that check must be same between this two methods.

This change came from #916 (diff)

@ewgRa Not sure what you meant with your second last sentence. Please open a pull request with some tests if possible for the suggested changes, so we can directly discuss it based on code.

Ok, I will write tests on this case and we will discuss.

stof · 2015-09-15T21:25:03Z

There is another thing which might not possible anymore: cloning a Query object (or any other objects impacted here): given you keep objects internally, most changes on 1 query could affect the other one too if you have any method mutating an inner object (I don't remember the API of all these classes enough to know whether this happens anywhere), while it would have worked before as arrays where used.
Would it make sense to implement __clone in these classes to clone the inner objects too ?

ewgRa · 2015-09-15T21:56:08Z

Nice notice. Looks like another projects clone all inner objects and this is a good practice and this make sense. Also this will be similar to how clone work before.

But, if it will be implemented via __clone, there is no possibility to clone just only object, without clone inner objects. And there is not a small chance that somebody need this feature.
And clone object with clone inner objects can be implemented via deepClone method, or similar.

I look to symfony code for example, and there is in many places __clone cause clone of inner objects. And how developer can just clone only object if needed?
Is there any good discussion about this?

If implement this, can it be considered as bugfix to 2.3.0? Because it is kind of broke BC :(

ewgRa · 2015-09-15T22:41:35Z

@ruflin @stof I think that object that can be set via setters can't be considered as "inner" objects and must be not cloned. Good example is symfony Request::duplicate and Request::__clone.
What is @stof meant - this is duplicate on my mind.

If for example Param::$_params will be an SplObjectStorage instance - than we must clone it, because for example $cloned->removeParam call must haven't affect on $original.

But because it is array now, we don't need to clone objects inside it.

So. I +1 for duplicate() when it will be requested by someone, but -1 for __clone() as duplicate().

ruflin · 2015-09-16T05:48:50Z

@stof Thanks for bringing up this discussion.

A few notes/thoughts here:

The good news is that we marked this change as BC break. The bad news is that there are very few people like @stof that read code changes and on top of it even realise the consequences. The part I'm most worried about about is if it will have any consequences to the usage of the FOSElasticaBundle. @stof @XWB can probably help here.
We should try to actively inform people about this change. I suggest to create more "explicit" release notes explaining the consequences. @ewgRa Can you help here?
For an implementation of __clone I suggest we wait until a specific problem with a use case pops up. As always, software is used in many ways and best is to implement by solving a problem. Then we can also check if we reached the goal.
Changes on one query should not change an other "independent" query as we don't work with singletons. Of course as soon the same object is passed to multiple queries, changes will have affect on all queries, which I think is also partially intented. The question is if this the affect and consequences are understood. Lets see if the topic "cloning" will pop up here.

ewgRa · 2015-09-16T06:56:49Z

Can you help here?

Ok, I will try to write something more verbose with examples and explanations.
But from 23 Sep I'm on vacation and will return 5 Oct. Don't know if I can finish it before vacation. If not - then only after 5 Oct. If it is urgent - better to ask someone else with help. Also I'm not a native English writer :)

stof · 2015-09-16T08:33:46Z

The good news is that we marked this change as BC break

@ruflin respecting semver, this means the release should have been numbered 3.0 btw.

ruflin · 2015-09-16T08:38:49Z

@stof Yep. If we are strict with semver ofr every BC break we must increase the full version. I normally sneak smaller BC breaks also into the MINOR releases. I think we should keep 3.0 for the changes to elasticsearch 2.0.

ruflin · 2015-09-17T13:04:34Z

@ewgRa Don't worry, most of us are not native english writers. Let me know in case you have a draft and I can take a look at it. Otherwise just drop me a line and will check if I can do it.

ewgRa · 2015-09-21T22:50:33Z

@ruflin
@im-denisenko

Article about broken BC: https://gist.github.com/ewgRa/217bb9226943309b1974

Please review, how it is looks like, is this exactly what needed, or not, and so on. Feel free to change, add you as co-author, publish and so on. I have vacation from 23 Sep and there is not so much time left to complete future requested changes in article. Do it yourself since my vacation will be started, or wait till I return from vacation.

ruflin · 2015-09-22T06:50:47Z

@ewgRa That is exactly what we needed. That is where we can point people to in case questions pop up. I revised the article and published it here on Elastica.io: http://elastica.io/2015/09/22/elastica-2-dot-3-0-broken-backward-compatibility-explanation/

It is linked in the release notes so it is easy to find. @ewgRa Thanks a lot and enjoy your vacations.

lazy toArray

c51800f

ewgRa mentioned this pull request Aug 13, 2015

Elastica/Query params converted to array on setters #783

Closed

im-denisenko reviewed Aug 14, 2015
View reviewed changes

ewgRa added 6 commits August 14, 2015 21:23

review fixes

2fd1361

review fixes

7270cc4

fix phpdoc

a33d068

getbasename optimization

9e5e5e6

cs fix

dd28aec

changelog change

4603f12

im-denisenko added a commit that referenced this pull request Aug 16, 2015

Merge pull request #916 from ewgRa/no-to-array-at-setters

c07e41a

lazy toArray

im-denisenko merged commit c07e41a into ruflin:master Aug 16, 2015

ewgRa deleted the no-to-array-at-setters branch August 16, 2015 21:07

ruflin mentioned this pull request Sep 10, 2015

Enable the support of "http.compression: true" in elasticsearch #927

Merged

stof reviewed Sep 15, 2015
View reviewed changes

This was referenced Sep 16, 2015

test that clone doesn't clone inner objects #934

Merged

fix empty string is not anonymous filter #935

Merged

xjewer mentioned this pull request Feb 17, 2016

fix phpdoc getQuery, it returns object instead array #1048

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lazy toArray #916

lazy toArray #916

ewgRa commented Aug 13, 2015

im-denisenko Aug 14, 2015

ewgRa Aug 14, 2015

im-denisenko commented Aug 14, 2015

ewgRa commented Aug 14, 2015

ruflin commented Aug 16, 2015

im-denisenko commented Aug 16, 2015

ruflin commented Aug 16, 2015

ewgRa commented Aug 16, 2015

ruflin commented Sep 15, 2015

ewgRa commented Sep 15, 2015

ruflin commented Sep 15, 2015

stof Sep 15, 2015

ruflin Sep 15, 2015

stof Sep 15, 2015

ruflin Sep 15, 2015

ewgRa Sep 15, 2015

ruflin Sep 16, 2015

ewgRa Sep 16, 2015

ruflin Sep 16, 2015

stof commented Sep 15, 2015

ewgRa commented Sep 15, 2015

ewgRa commented Sep 15, 2015

ruflin commented Sep 16, 2015

ewgRa commented Sep 16, 2015

stof commented Sep 16, 2015

ruflin commented Sep 16, 2015

ruflin commented Sep 17, 2015

ewgRa commented Sep 21, 2015

ruflin commented Sep 22, 2015

lazy toArray #916

lazy toArray #916

Conversation

ewgRa commented Aug 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

im-denisenko commented Aug 14, 2015

ewgRa commented Aug 14, 2015

ruflin commented Aug 16, 2015

im-denisenko commented Aug 16, 2015

ruflin commented Aug 16, 2015

ewgRa commented Aug 16, 2015

ruflin commented Sep 15, 2015

ewgRa commented Sep 15, 2015

ruflin commented Sep 15, 2015

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

Choose a reason for hiding this comment

stof commented Sep 15, 2015

ewgRa commented Sep 15, 2015

ewgRa commented Sep 15, 2015

ruflin commented Sep 16, 2015

ewgRa commented Sep 16, 2015

stof commented Sep 16, 2015

ruflin commented Sep 16, 2015

ruflin commented Sep 17, 2015

ewgRa commented Sep 21, 2015

ruflin commented Sep 22, 2015