NEW: Add ViewableData::setFailover() to refresh detected methods when changing failover #4870
Conversation
| */ | ||
| protected function checkAndRedefineMethods() { | ||
| if ($this->failover !== $this->oldFailover) { | ||
| $this->oldFailover = $this->failover; |
There was a problem hiding this comment.
I did consider using spl_object_hash() for comparing the failover objects (instead of keeping two copies of them in memory), but I don’t think it’s reliable enough - the same hash can be re-used once an object is destroyed.
There was a problem hiding this comment.
If you force defineMethods() every time setFailover() or __construct() is called, would this remove the need for oldFailover check?
There was a problem hiding this comment.
Indeed it would, I think that’ll be the best approach :)
|
Here's a small critique; Will we ever need to re-generate the extra methods when assigning a different instance of a failover of the same class? It looks like the instance is extracted from the failover in You might also need a mechanism and test to ensure you remove methods from the removed failover, since it looks like the new ones are being merged with the old. |
| * @inheritdoc | ||
| */ | ||
| public function hasMethod($method) { | ||
| $this->checkAndRedefineMethods(); |
There was a problem hiding this comment.
Maybe this should be on setFailover instead, rather than duplicating it over all methods that rely on the failover?
There was a problem hiding this comment.
There is no setFailover() currently (see original comment) :(
There was a problem hiding this comment.
+1. If you have it all over the place (especially in __call()) then its going to redefine all methods every time something on the failover object changes.
It’s unlikely, but given that extra methods are defined at an instance-level instead of statically it’s possible, which is why I used that approach. Upon reflection, I’m wondering if it might be better to add a |
| parent::__construct(); | ||
| } | ||
|
|
||
| public function setFailover($failover) { |
There was a problem hiding this comment.
This is the setFailover I was refering to. :)
There was a problem hiding this comment.
Ah yes, I only added that to the test class. I think I’ll redo this PR and add it to ViewableData, we should probably have a setter anyway.
|
Ok, thanks for your response. :) I'll have a look once you have an update. |
kinglozzer
force-pushed
the
4821-viewabledata-failover
branch
from
8c04ff7
to
9d076da
Compare
… changing failover
kinglozzer
force-pushed
the
4821-viewabledata-failover
branch
from
9d076da
to
c9ba0e4
Compare
kinglozzer
changed the title
|
Okay, updated. This PR is looking a bit simpler now :). I’ll do a PR for cms to go with this one that actually fixes #4821 Edit: CMS PR is here - silverstripe/silverstripe-cms#1355 |
NEW: Add ViewableData::setFailover() to refresh detected methods when changing failover
Hopefully the tests explain this better than my bug ticket did...
This is the only workaround I could think up for this issue. We don’t have a setter like
ViewableData::setFailover()to hook into, and even if we added one we have no guarantee that people would use it (especially because setting$this->failover = $foo;has been an undocumented convention for a while now).