laminas
reduce arguments by reference
This PR reduces arguments by reference often used on internal methods.
It was previously done as performance improvement but in fact it decreases performance as PHP have to create a new zval and in some cases it needs to copy the value before PHP-7 as described here: http://nikic.github.io/2015/05/05/Internal-value-representation-in-PHP-7-part-1.html
Additionally arguments by reference make the code more error prone and hard to read.
Originally posted by @marc-mabe at https://github.com/zendframework/zend-cache/pull/8
I'll suggest to open a issue for each idea so we could discuss over each one separately.
Originally posted by @Maks3w at https://github.com/zendframework/zend-cache/pull/8#issuecomment-111712013
@Maks3w I have changed the title and description on this PR. Will open new for the other ideas.
Originally posted by @marc-mabe at https://github.com/zendframework/zend-cache/pull/8#issuecomment-111746599
@marc-mabe Thanks. Tiny objectives make it more easy to be approved and merged.
Originally posted by @Maks3w at https://github.com/zendframework/zend-cache/pull/8#issuecomment-111754724
related to #67
Originally posted by @marc-mabe at https://github.com/zendframework/zend-cache/pull/8#issuecomment-200994906
I had hoped that the fix for this (#295) would have fixed https://github.com/laminas/laminas-cache-storage-adapter-memory/issues/5 as well but unfortunately it didn't.
I think the main reason is that unlike the original https://github.com/zendframework/zend-cache/pull/8, #295 did not remove/replace ArrayObject by array (https://github.com/zendframework/zend-cache/pull/8/commits/f3d97933a004200e274a45fa1f29a03f4a06f1b1).
Shall I open a separate issue for that @boesing?
The changes of #295 are just merged but there is no release yet, so ofc. it does not fix anything unless you somehow managed to consume the 4.0.x branch with a modified branch of the memory adapter on your own.
Regarding replacement of ArrayObject with array - that won't happen as most of the event listeners are actually modifying both values and keys. Having just an array wont provide a way to have the modified key(s)/value(s) being received after triggering events.
The only way I could think of to handle this kind of thing would be to introduce dedicated event objects for every event.
But that will not be part of v4.0.0 of laminas-cache. You can, however, create a new RFC where this can be discussed.
The changes of #295 are just merged but there is no release yet, so ofc. it does not fix anything unless you somehow managed to consume the 4.0.x branch with a modified branch of the memory adapter on your own.
You're a funny guy... Of course I used the 4.0.x branch together with a version of the memory adapter with all the necessary signature changes applied for my test.
Regarding replacement of
ArrayObjectwitharray- that won't happen as most of the event listeners are actually modifying both values and keys. Having just anarraywont provide a way to have the modified key(s)/value(s) being received after triggering events.
Oh dear... Well, I don't know any details of the design choices of this particular lib so I cannot comment on that. Just surprises me that this was part of the original PR and no-one seems to have objected that then.
The only way I could think of to handle this kind of thing would be to introduce dedicated event objects for every event. But that will not be part of v4.0.0 of
laminas-cache. You can, however, create a new RFC where this can be discussed.
I guess if I ever need to have the functionality in question fixed I'd rather try and find another PSR-16 implementation that works or write my own. Unfortunately I don't have the time to deep-dive into the constraints that are particular to this library, but I do understand that it covers much more than just my use case.
Replacing laminas-cache with something else is ofc always a solution.
However, I am still unsure if the ArrayObject, which is passed to the events and destroyed after returning from whatever method was invoked, does actually lead to the "memory leak" we are talking about in your issue.
Regarding the "initial PR" in question: that is almost 10 years old, I haven't spent a single second to dive into those changes.
The main idea of this issue and the "initial PR" was to remove pass-by-reference of arguments, which was resolved with #295.
Having an ArrayObject is not passing arguments by reference and unless there is an internal PHP bug which treats ArrayObject other than a class containing an array property, that should not introduce any "memory leak" as reported in the memory adapters issue. If PHP does treat that ArrayObject different (which is destroyed after returning from any method using it), I would expect PHP to fix that as that would be a problem not just with the cache component in question.
Since you already adapted some changes locally to have a compatible version with the recent 4.0.x branch, could you verify that once using array over ArrayObject in your use-case does fix the memory leak? If it does, I am happy to remove ArrayObject as well. If not, I'd rather keep it for the sake of BC.
I was unable to find a connection between the use of ArrayObject and the memory adapter issues. Instead I now suspect that it's an issue with the Composer autoloader. See my latest comment in laminas/laminas-cache-storage-adapter-memory#5 for details. So at least for now I'm resting my case concerning laminas-cache internals causing the issue over there.