#019400: Specify db instance to use for eZPersistentObject

[lafka]

Allow to use different database for eZPersistentObject

See http://issues.ez.no/IssueView.php?Id=19400 for more information

[gggeek]

Interesting (and welcome)

Just one thing: if we create an object from db X, when we update/remove it, should it not remember by itself which db it came from? Otherwise the code risks trying to update/remove it from a different db...

That would mean storing within the object itself a reference to the source db though, which I'm not entirely sure it's a good idea...

[lafka]

Your spot on, i considered adding an internal pointer to eZDB. It could be implemented by adding it as a parameter in the construct; the downside is you then have all the strict error's that would be triggered as the parameters no longer match in eZPersistentObject and any sub-class.

On the other hand storing the DB reference is no difference from all the calls to eZDB::instance, this could the be manipulated through get/set methods.

The use case for me is a race condition in master/slaves setup. When a user registers certain triggers are not completed because the object can't replicate to all the slaves in time and therefor cannot be found. This causes user don't get initial roles & policies set. This is fixed by forcing to read from master.

I would love to hear more thoughts on this

[andrerom]

Could you elaborate on the use case a bit?

If this is your custom data, then I would say using Zeta Persistence or something else might work better and more easily allow you to do such a thing. If I where to add this I think I would have created a function ->getDBInstance() that all persistence objects have to use, and make it possible to provide a custom one in constructor. But as you both know, that would be a BC break.

[gggeek]

Mmm, it turns out to be more complex than anticipated:

• if we patch constructor, we get warnings from php for classes that reimplement it
• if we patch only a couple of methods in the base class, all the subclasses will not be able to take advantage of this in an automatic way, as the fetchThis() in a derived class does not know that it can pass a new param to the call parent::fetchobject() that it does internally
• if we don't patch the fetch methods, code gets a much more messy: we could implement a new "setDBToUse" method in ezpo, which sets the current db to be a different one. Then the next fetch executed would run on the new db, (and possibly reset to std db to avoid unexpected consequences). Of course, this could be implemented on the db layer as well instead of the ezpo layer...

[andrerom]

if we patch constructor, we get warnings from php for classes that reimplement it

Sure? I coud not find a singel class that re implements eZPersistentObject() function. And even if it was named __construct(), as long as it is not defined in interface or as abstract, then changing signature in descending classes is allowed.

So something like this can be a good start:

diff --git a/kernel/classes/ezpersistentobject.php b/kernel/classes/ezpersistentobject.php
index 8411fdd84a556f611144bddeb91580fe64326cbe..feab02a189188a8210c32ddf1b4735029c11c4cf 100644
--- a/kernel/classes/ezpersistentobject.php
+++ b/kernel/classes/ezpersistentobject.php
@@ -42,6 +42,11 @@ class eZPersistentObject
     public $PersistentDataDirty;

     /**
+     * @var eZDBInterface
+     */
+    private $dbInstance;
+
+    /**
      * Initializes the object with the $row.
      *
      * It will try to set each field taken from the database row. Calls fill
@@ -49,13 +54,29 @@ class eZPersistentObject
      * database using it as the unique ID.
      *
      * @param int|array $row
+     * @param \eZDBInterface|null $customDbInstance Makes it possible to specify another db instance
+     *        then the global one. Only supported by persistence objects that use {@link getDbInstance()}
+     *        instead of {@link eZDB::instance()} direclty. Available since 4.7.
      */
-    public function eZPersistentObject( $row )
+    public function eZPersistentObject( $row, eZDBInterface $customDbInstance = null )
     {
         $this->PersistentDataDirty = false;
         if ( is_numeric( $row ) )
             $row = $this->fetch( $row, false );
         $this->fill( $row );
+
+        if ( $customDbInstance !== null )
+            $this->dbInstance = $customDbInstance;
+        else
+            $this->dbInstance = eZDB::instance();
+    }
+
+    /**
+     * @return eZDBInterface
+     */
+    protected function getDbInstance()
+    {
+        return $this->dbInstance;
     }

     /**

But it does indeed not cover static fetch functions.. :(

[lafka]

@andrerom The issue we have is further explained here http://issues.ez.no/IssueView.php?Id=19404

The alternative is using eZDB::setInstance(); which is no better, i tried hacking a solution around it and ended up with table lock from 1 db instance that the second instance did not know about resulting in a deadlock.

I need to do some more research on the side effects of switching database instance in the middle of runtime.

[andrerom]

ah, ok, then pull #339 or something like it is a better way to fix it I would say.

[lafka]

if we patch constructor, we get warnings from php for classes that reimplement it

Sure? I coud not find a singel class that re implements eZPersistentObject() function. And even if it was named > __construct(), as long as it is not defined in interface or as abstract, then changing signature in descending classes is allowed.

True, so would this be a viable solution? considering your patch and switching all the eZDB::instance() calls to use the $dbInstance

I guess the question is: What is the possible sideeffects? A quick search eZDBGlobalInstance says that there is no uses of it outside the eZDB class so it should be safe.

[lafka]

Updated with @andrerom updates on the __construct and eZPersistentObject::getDbInstance

[andrerom]

Thanks, but it does not cover static fetch functions. And if the use case is indeed master/slave, then that should be handled on db layer, not model layer + custom business logic.

[lafka]

Would you like to keep the eZDBInterface instance in the static method calls?

The initial usecase is not there anymore, but i still find this usefull as it lets me use ezpo classes with other databases.

2012/5/7 André R. < [email protected]

Thanks, but it does not cover static fetch functions. And if the use case is indeed master/slave, then that should be handled on db layer, not model layer + custom business logic.

---

Reply to this email directly or view it on GitHub: https://github.com/ezsystems/ezpublish/pull/337#issuecomment-5550906

Med Vennlig Hilsen Olav Frengstad

Systemutvikler // FWT +47 920 42 090

[andrerom]

If there is no use cases left then I would prever to close this pull request as it adds extra complexity to an already complex class :)

[gggeek]

The use case of using ezpo for remote db stuff is imho real - I think the feature request for that has been in with for a while...

[andrerom]

Sure, and it should be supported in 5.x; given that most things are dependency injected there it makes it much easier and cleaner to accomplish such a thing.

[lafka]

I do agree with Andre considering the timeframe it will be available before 5.x is released, i don't think it will be picked up before people start using 5.x. On the other hand there will be plenty of 4.x sites that will most likely never be upgraded but might get new features where this pull request MAY be of us.