zendframework / zend-expressive-session

Session middleware for PSR-7 applications
BSD 3-Clause "New" or "Revised" License
27 stars 11 forks source link

add short-circuit return for isRegenerated #34

Closed pine3ree closed 5 years ago

pine3ree commented 5 years ago
weierophinney commented 5 years ago

Thanks, @pine3ree! Cherry-picked to master for release with 1.2.1.

pine3ree commented 5 years ago

Just ot of curiosity...my usual silly question (@weierophinney ) why couldn't we just delegate hasChanged() call directly to the proxied session (if existing) instead of calling the proxy's isRegenerated() method as well? https://github.com/zendframework/zend-expressive-session/blob/695fe2e7528ae0d52ef2b5c8c58415ff1fa52798/src/LazySession.php#L101 The proxy method checks by itself the proxy's isRegenerated property. https://github.com/zendframework/zend-expressive-session/blob/695fe2e7528ae0d52ef2b5c8c58415ff1fa52798/src/Session.php#L122 From this test though https://github.com/zendframework/zend-expressive-session/blob/695fe2e7528ae0d52ef2b5c8c58415ff1fa52798/test/LazySessionTest.php#L151 it seems we don't want to call the proxy hasChanged() method. The proxy is encapsulated so the only way to get a regenerated clone it is to call the lazy wrapper regenerate() method.

In the current implementation, assuming that the proxied session has been instantiated

$lazySession->hasChanged()
>>calls
$proxy->isRegenerated()
>>which internally checks the
$proxy->isRegenerated property
>>if false, calls
$proxy->hasChanged()
>>which internally checks (again) the
$proxy->isRegenerated property
>>if false (again, if it was before it is now) finally
>>compares data with original data

kind regards

PS I didn't change that behaviour in the PR because of the test case, but I couldn't find a good reason for it