New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FEATURE: Fusion performance optimization (lazy Component props) #2738
Changes from 4 commits
b7f9da9
ad868a7
0f86e14
4e61359
aa168b7
db7c111
c652338
9fcaa94
3544bef
71b230d
0642115
78f0b14
1c61b5d
6982fb6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,117 @@ | ||
<?php | ||
|
||
namespace Neos\Fusion\FusionObjects\Helpers; | ||
|
||
use Neos\Flow\Annotations as Flow; | ||
use Neos\Fusion\Core\Runtime; | ||
|
||
/** | ||
* @Flow\Proxy(false) | ||
*/ | ||
final class LazyProps implements \ArrayAccess, \Iterator | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it make sense to add this as As this is no @api yet it also makes sense to think about moving it later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about it too, but this is rather specific for holding an array of keys and defering the evaluation of these. I'm not sure where and how we would re-use this. By only checking the interfaces in Runtime we always have the chance to extract a common class or something else later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. :-) i now have a usecase for that. Evaluating only the current page of a multipage form. But since i the fusion-forms shall support Neos 4.3 i will copy that code anyways but thanks for the inspiration. |
||
{ | ||
|
||
/** | ||
* @var array | ||
*/ | ||
private $valueCache = []; | ||
|
||
/** | ||
* @var string | ||
*/ | ||
private $parentPath; | ||
|
||
/** | ||
* @var Runtime | ||
*/ | ||
private $runtime; | ||
|
||
/** | ||
* Index of keys | ||
* | ||
* @var array | ||
*/ | ||
private $keys; | ||
|
||
/** | ||
* @var object | ||
*/ | ||
private $fusionObject; | ||
|
||
/** | ||
* @var array | ||
*/ | ||
private $effectiveContext; | ||
|
||
public function __construct( | ||
object $fusionObject, | ||
string $parentPath, | ||
Runtime $runtime, | ||
array $keys, | ||
array $effectiveContext | ||
) { | ||
$this->fusionObject = $fusionObject; | ||
$this->parentPath = $parentPath; | ||
$this->runtime = $runtime; | ||
$this->keys = array_flip($keys); | ||
$this->effectiveContext = $effectiveContext; | ||
} | ||
|
||
public function offsetExists($path) | ||
{ | ||
return isset($this->keys[$path]); | ||
} | ||
|
||
public function offsetGet($path) | ||
{ | ||
if (!isset($this->valueCache[$path])) { | ||
$this->runtime->pushContextArray($this->effectiveContext); | ||
try { | ||
$this->valueCache[$path] = $this->runtime->evaluate($this->parentPath . '/' . $path, | ||
$this->fusionObject); | ||
} finally { | ||
$this->runtime->popContext(); | ||
} | ||
} | ||
return $this->valueCache[$path]; | ||
} | ||
|
||
public function offsetSet($path, $value) | ||
{ | ||
// NOOP | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't that throw? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mean the "set call" should throw an exception? Yes, we could do that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still think trying to set a lazy prop should throw :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, let's do that IMO. Otherwise looks good :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @albe It's a good idea to resolve all conversations before hitting the merge button. Otherwise open discussions might get lost – like this one.. :) |
||
} | ||
|
||
public function offsetUnset($path) | ||
{ | ||
// NOOP | ||
} | ||
|
||
public function current() | ||
{ | ||
$path = key($this->keys); | ||
if ($path === null) { | ||
return null; | ||
} | ||
return $this->offsetGet($path); | ||
} | ||
|
||
public function next() | ||
{ | ||
next($this->keys); | ||
} | ||
|
||
public function key() | ||
{ | ||
return key($this->keys); | ||
} | ||
|
||
public function valid() | ||
{ | ||
return current($this->keys) !== false; | ||
} | ||
|
||
public function rewind() | ||
{ | ||
reset($this->keys); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Is this supposed to be extended/inherited and hence the added typehints to be considered breaking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not @api and no intended extension point. We only seperated this into a special method so we can address this in aspects seperately.
So for me this is ok.