flow-development-collection
flow-development-collection copied to clipboard
neos
BUGFIX: isNewObject
With this change PersistenceManager::isNewObject will correctly report object status in all circumstances.
Fixes: #2699
This will report "false" on unmanaged new Entities. Shouldn't we take the extra mile and provide the call with a custom state like "UNKNOWN" and try to see if the entity is fetchable from persistence to replace guessing with knowledge?
Without assumption doctrine will look into the DB, and it will only ever come back with DETACHED or NEW, so that seems alright to me?
Oh? Ok... obiously it is easy to make false assumptions. ;)
I would never have thought, that Doctrine would report DETACHED for unknown entities, but well... fine then.
From my understanding of the code it will report DETACHED if the entity is found in the DB but was not in the unit of work (which seems the correct info then) and otherwise it would report NEW, which would also seem correct.
I am actually more concerned about the test failures because the seem rather unrelated? I will have a look locally....
These test failures actually expose a problem in our ValueObject implementation (in combination with embedded=true I think). We never went beyond the first 5 lines of \Doctrine\ORM\UnitOfWork::getEntityState due to the $assume parameter. Now UoW tries to call \Doctrine\ORM\Mapping\ClassMetadataInfo::getIdentifierValues() on the Location ValueObject used as Fixture in these tests. That results in an exception that is hidden away due to the functinoal test and using browser requests:
Uncaught Exception in Flow #1: Warning: Undefined array key 0 in /Volumes/CaseSensitive/Work/PhpstormProjects/neos-dev-73/Packages/Libraries/doctrine/orm/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php line 940
thrown in file Packages/Framework/Neos.Flow/Classes/Error/ErrorHandler.php
in line 81
65 Neos\Flow\Error\ErrorHandler::handleError(2, "Undefined array key 0", "/Volumes/CaseSensitive/Work/PhpstormProjects/neos-…orm/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php", 940)
64 Doctrine\ORM\Mapping\ClassMetadataInfo::getIdentifierValues(Neos\FluidAdaptor\Tests\Functional\Form\Fixtures\Domain\Model\Location)
63 Doctrine\ORM\UnitOfWork::getEntityState(Neos\FluidAdaptor\Tests\Functional\Form\Fixtures\Domain\Model\Location)
62 Neos\Flow\Persistence\Doctrine\PersistenceManager_Original::isNewObject(Neos\FluidAdaptor\Tests\Functional\Form\Fixtures\Domain\Model\Location)
61 Neos\FluidAdaptor\ViewHelpers\Form\AbstractFormViewHelper::renderHiddenIdentityField(Neos\FluidAdaptor\Tests\Functional\Form\Fixtures\Domain\Model\Location, "post[author][location]")
60 Neos\FluidAdaptor\ViewHelpers\Form\AbstractFormFieldViewHelper::addAdditionalIdentityPropertiesIfNeeded()
59 Neos\FluidAdaptor\ViewHelpers\Form\TextfieldViewHelper_Original::render()
I assume ValueObjects never come by said method usually. I wonder if we should catch them as special case in isNewObject because we can't even know the state for an embedded ValueObject like this...
I find \Neos\FluidAdaptor\ViewHelpers\Form\AbstractFormViewHelper::renderHiddenIdentityField a bit "loose" in that it just asks for isNewObject on any object, regardless if we know if it's even a ORM managed object. BUT I am also unclear how we can detect one from the other. Figuring out if a class is annotated ValueObject(embedded=true) seems rather cumbersome.
Well... every PersistenceObject should get the PersistenceMagicAspet introduced, if I understand it correctly.
That way we should be able to ask it, what kind of object it is, shouldn't we?
Well... every PersistenceObject should get the PersistenceMagicAspet introduced, if I understand it correctly.
That way we should be able to ask it, what kind of object it is, shouldn't we?
facepalm, good call, that should do the trick. I'll try.
I guess we should technically type hint PersistenceMagicInterface for the methods in the PersistenceManager, would be breaking but maybe something for 9.0 ?