Remove pass by reference in NodeSearch plugin

Looks fine. Bot likes it. No downside.

Parent issue was committed to 8.8. 8.9, and 9. This one probably should be too.

@andypost brought up in Slack:

Not sure that cloning array is more efficient comparing to passing by reference

I think the reason this was suggested by @larowlan is related to his comment when suggesting pass-by-reference be removed from the "grandparent" issue #3075695: Move search_index() and related functions to a service:

Is there a reason why we can't make this a boolean, instead of a pass-by-reference, which is a bit of a code smell.

So, there are two considerations:
a) Do we for some reason not like pass-by-reference ("code smell")?
b) Do we think that pass-by-reference is more/less efficient than returning an array and doing array addition?

So the two patterns are:
1.

$foo += return_some_array_values($bar);

2.

add_in_the_array_values($bar, &$foo);

Which is actually better, less smelly, and more efficient? If 2 is better, we should go back to the grandparent issue and put back in the pass by reference. If 1 is better, we should mark this one as RTBC.

I ran a quick test with the attached PHP script. The pass-by-reference pattern is slightly faster than the plus-equals pattern -- about 15%. I'm not sure we care about efficiency, but that is what my test says. Not sure how it scales.

I would expect the by-ref version is slightly faster since you are only updating one array, vs doing the updates 2x.

I don't have a strong feeling about which is "cleaner". I think the current code is really fine since this is all happening within the one object instance.

Well the other question is should we go back to #3075695: Move search_index() and related functions to a service and put the pass-by-reference into the API, as was suggested by earlier patches. But maybe we don't want a pass-by-reference in an API function like that?

Interesting. So the relative speed may depend on which version you run on. It's never very much difference anyway -- we should probably just go by coding style preference?

In which case, I think the patch is probably a good idea, since it mirrors what the search API is doing.

However, it doesn't seem quite right, as it is. The $words array is passed into the new method, modified there, and returned. But then the return value of $this->indexNode() is added to the previous $words array.

So we should either:
a) Not pass $words in to indexNode() -- similar to what the search API is doing -- and use $words += return value
b) Pass $words in to indexNode() by reference (no patch, the current code)
c) Pass $words in by value, modify it there, and use $words = return value

I think option a) is best if it's a public api. b) is fine too, especially for non-public. c) doesn't make sense

It is not really a public API, is it? This issue is about modifying a protected method on NodeSearch.

My preference is for option a)

+1 rtbc if green

Looks good. Let's get it in before 8.8.0, to go with #3087407: Search indexing calls are inefficient, which already changed this method anyway.

Apologies for the nuisance, but I think we can't change the signature here, in case of sub-classes (yes protected methods are internal etc but we should still care about the minor upgrade path) - see https://3v4l.org/tVjks

In light of that, I'm inclined to mark this closed won't fix - I don't think its worth the effort/disruption - sorry 😬

What do people think?

Ummmm... Well, you already (4 days ago) committed #3075695: Move search_index() and related functions to a service, which changed the function signature:

-  protected function indexNode(NodeInterface $node) {
+  protected function indexNode(NodeInterface $node, array &$words) {

This patch changes it further to

+   * @return array
+   *   An array of words to update after indexing.
    */
-  protected function indexNode(NodeInterface $node, array &$words) {
+  protected function indexNode(NodeInterface $node) {

Which is actually closer to what it was, although there is a new contract that the caller needs to deal with the returned words.

So... this should be considered (I think) a quick update to what was already a BC break in that other issue.

oh! so this changes it back - and removes the minor BC break - perfect!

Committed 89e11e4 and pushed to 9.0.x. Thanks!

c/p to 8.9 and 8.8