[WIP] Removed extra event classes, made more similar to Commons 2.4 #248

Closed
wants to merge 2 commits into
from

Projects

None yet

4 participants

@dantleech
Contributor

This is really a PR to continue the discussion started at #246.

Here I have:

  • Removed the duplicated events and replaced them with ManagerEventArgs
  • Added getObject and getObjectManager in addition to getDocument and getDocumentmanager
@stof stof commented on an outdated diff Feb 21, 2013
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -42,6 +41,7 @@
use PHPCR\UnsupportedRepositoryOperationException;
use PHPCR\RepositoryException;
use PHPCR\Util\UUIDHelper;
+use Doctrine\ODM\PHPCR\Event\ManagerEventArgs;
@stof
stof Feb 21, 2013 Member

Where is this class ?

thus, you should put the use statement with the other ones

@dbu dbu and 1 other commented on an outdated diff Feb 23, 2013
...ctrine/ODM/PHPCR/Event/LoadClassMetadataEventArgs.php
@@ -66,4 +66,14 @@ public function getDocumentManager()
{
return $this->dm;
}
+
+ /**
+ * Retrieve associated ObjectManager.
+ *
+ * @return \Doctrine\PHPCR\ODM\ObjectManager
@dbu
dbu Feb 23, 2013 Member

even though its called ObjectManager, this is the DocumentManager. The ObjectManager is a class from commons

@dbu
dbu Feb 23, 2013 Member

here we should extend https://github.com/doctrine/common/blob/master/lib/Doctrine/Common/Persistence/Event/LoadClassMetadataEventArgs.php (if we need to add anything, otherwise just drop the class)

@dantleech
dantleech Mar 1, 2013 Contributor

Do we want to have the getDocument and getDocumentManager methods? I am thinking "yes" and assuming that we will replace all Document/DocumentManager references in the ODM with Object/ObjectManager at some point .. is that the plan? /cc @stof, @lsmith77

@dbu dbu commented on an outdated diff Feb 23, 2013
lib/Doctrine/ODM/PHPCR/Event/LifecycleEventArgs.php
@@ -49,4 +49,20 @@ public function getDocumentManager()
{
return $this->dm;
}
+
+ /**
+ * @return object
+ */
+ public function getObject()
@dbu
dbu Feb 23, 2013 Member

i think we should at least deprecate getDocument and getDocumentManager methods. and drop them before we release 1.0. we don't want to carry a bag of legacy before we even have the first release :-)

@dbu
dbu Feb 23, 2013 Member

and we must extend the commons LifecycleEventArgs here. this will give us getObjectManager and getObject methods for free. https://github.com/doctrine/common/blob/master/lib/Doctrine/Common/Persistence/Event/LifecycleEventArgs.php

@dbu
Member
dbu commented Feb 23, 2013

in commons 2.4 there are base classes for LifecycleEventArgs and LoadClassMetadata - we should use those, or extend if we have additional things.

i merged the move events in #246 - can you rebase this branch on master and make those LifecycleEventArgs with additional getSourcePath and getTargetPath methods? then we would have getObject and getObjectManager for free again...

@lsmith77
Member

ping

@dantleech
Contributor

forgot about this - havn't got any time today but should be able to sort it out tomorrow.

@dantleech
Contributor

updated. have rebased and extended the Doctrine commons events and added proxies for getDocument/getDocumentManager with deprecation comments.

@stof stof commented on the diff Mar 1, 2013
lib/Doctrine/ODM/PHPCR/Event/LifecycleEventArgs.php
*/
public function getDocument()
{
- return $this->document;
@stof
stof Mar 1, 2013 Member

@lsmith77 @dbu What do you think about triggering a E_USER_DEPRECATED (similar to what Symfony 2.2 does) ?

@dbu
dbu Mar 1, 2013 Member

i think it makes sense, as we will drop it in a month anyways.

@stof stof commented on the diff Mar 1, 2013
lib/Doctrine/ODM/PHPCR/Event/LifecycleEventArgs.php
*/
public function getDocument()
{
- return $this->document;
+ return $this->getEntity();
@stof
stof Mar 1, 2013 Member

This should be getObject, not getEntity

@dbu
dbu Mar 1, 2013 Member

depends if we require commons 2.4 or commons 2.3.

if we would know if orm 2.3 will be compatible with commons 2.4 i would go for that immediately. but if not, we would prevent using orm 2.3 in the same project with phpcr-odm which would be bad, given that it might take some while until orm 2.4 becomes stable

@dbu
Member
dbu commented Mar 1, 2013

cool, thanks a lot. if @beberlei could shed some light on the orm 2.3 with commons 2.4 question, we can merge this (and #214 too if orm 2.3 is compatible with commons 2.4). if not, i suggest we implement a getObject method ourselves here until we drop commons 2.3 support.

@dantleech you tested and built against commons 2.3, right? did you ever try if it also works with the current master of commons?

@stof
Member
stof commented Mar 1, 2013

If you want to keep the compatibility with Common 2.3, you should indeed implement getObject for 2.3 to be sure it is there (otherwise, people would not be able to use it). But be careful when implementing this forward compatibility method to avoid triggering some infinite loop in 2.4

@dantleech
Contributor

@dbu I get an error about the Annotation class on commons/master:

phpcr-odm git:(event_class_refactor) ✗ phpunit -c tests/phpunit_jackrabbit.xml.dist tests/Doctrine/Tests/ODM/PHPCR/Event
PHP Fatal error:  Class 'Doctrine\Common\Annotations\AnnotationRegistry' .... ( not found )
@stof
Member
stof commented Mar 1, 2013

@dantleech in master, the annotation library has been moved from Common to a separate repo (required by Common in composer.json). So be careful if you are not using composer. You will need to get the new repos too

@dbu
Member
dbu commented Mar 1, 2013

ah yes, they got refactored out into their own repository to be used independently of doctrine. lets see if benjamin can clarify about 2.4 before doing anything more.

@dbu dbu was assigned Apr 18, 2013
@dbu
Member
dbu commented May 26, 2013

superseeded by #288

@dbu dbu closed this May 26, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment