Revert "Alter guarantees provided by StatementList::toArray"

[thiemowmde]

Reverts #459. I don't understand why #459 was commited and merged without referencing the not finished discussion in #446.

[JanZerebecki]

I'd prefer the discussion is resolved before a 3.0.0 release, but if it will not be then I'd merge this.

[JeroenDeDauw]

-2.

@thiemowmde I'm not sure what you do not understand about the change. The problem was outlined and other people seem to understand it.

[thiemowmde]

Discussion continues at #446 (comment).

[JeroenDeDauw]

Dropping my -2 as per discussion. -1 now, assuming the author and merger take responsibility for any problems that this causes with adding removal methods to StatementList.

[Benestar]

When introducing a removeStatement method which is one option per #455 we have to call array_values in this method. Just consider that pls.

[JeroenDeDauw]

@Benestar that is exactly the concern I've been outlining.

With the removal functionality, we will need to do a reindex somewhere. My suggestion is that users of toArray that care about the array being non-sparse should take care of that themselves, assuming there are very few such users. Thiemos suggestion is that StatementList takes care of reindexing, whenever a statement is removed.

per #455

Linked the wrong issue?

[Benestar]

per #455

Linked the wrong issue?

Actually nope. From that issue:

Other options would be

1. Create another interface which inherits StatementListProvider and includes the setter. Possible but maybe too much overhead
2. Add a method to StatementListto clear that list or remove particular statements from that list. However, this would ignore "Does not provide complex modification functionality."

I agree with you that StatementList itself shouldn't take care of reindexing for performance reasons. Methods where a non-sparse array is required can do that themselves by calling array_values or using another class which may be more appropriate like ByPropertyIdGrouper in the Serialization package. The performance overhead created in foreach loops for example isn't worth that guarantee imo.

[Benestar]

+1 – the cost of the array_values call is no performance issue in this place because removeStatementWithGuid won't be called that often that it will hit any performance problems.

[JeroenDeDauw]

I'll merge this now, even though I do not agree with it, as otherwise we are leaving much more serious issues unattended. I'm quite disappointed this is needed to get things done.

[thiemowmde]

This removes a breaking change from a release and reverts it back to the less error-prone status quot. I'm reading your comment as if the status quo causes or is an "issue", but can't find substantial arguments in #446 other than a suspected performance issue. What am I missing?

[JeroenDeDauw]

May I remind you that originally this method did not have a non-sparseness guarantee?

[thiemowmde]

#311, #322. Quote: "StatementList always reindexed the array. It stopped doing this [in] 2.4.0".

[JeroenDeDauw]

I'm talking about the guarantees provided by the method

[thiemowmde]

What are we discussing here? Again? The class always gave this guarantee.