Closed soerenbernstein closed 8 years ago
So, is the current url missing a parameter?
Seems that way but I've no idea why. I checked against the older version and the parameters seems to be the same. The field_description.options.route.parameters in SonataORMAdminBundle:CRUD:show_orm_many_to_one.html.twig are empty in both SonataAdmin 3.x and 2.3.
Also, I find it rather strange to have links to edit an associated entity while showing an old revision. In my mind, this should only show the state of the entity without any kind of editing. And the link is going to the current version of the associated entity, not the version of the entity valid for the selected revision.
So, is the current url missing a parameter?
Out of curiosity, can you show us the current url (which should be missing a parameter ?) Does the symfony debugger show the value of each parameter properly or not?
I'm not quite sure how? The url in questing would be in the result which I can't get because of the exception.
Oh right, it's a generated url actually, it's not a problem with the currenty url, my bad.
Look at the stack trace. The problem might be that your Company
object has a null id. Can you confirm that?
I've commented the url generation in the template and dumped the parameters involved in generating the url. Either calling it 'directly' as a show action or 'indirectly' in the history context, the parameters are the same - as far as I can see.
No, the company has an id.
Company {#6976 ▼
#name: "*******************"
#status: "A"
#locked: false
#deleted: false
#timeCreated: DateTime {#6977 ▶}
#timeActivated: null
#timeDeleted: null
#timeLastOn: DateTime {#6978 ▶}
#latitude: null
#longitude: null
#street: "****************************"
#postalCode: null
#city: null
#district: null
#contact: null
#email: null
#phone: null
#maxUsers: null
#maxUsersInfo: null
#registrationFee: null
#registrationFeeBegin: null
#activationTemplate: null
#activationTemplateMode: null
#note: null
#pass: "Wellnet"
#canCancelCustomers: true
#selfAdministered: true
#users: ArrayCollection {#6981 ▶}
#employees: AuditedCollection {#6979 ▶}
#contracts: AuditedCollection {#6980 ▶}
#id: 254
-changedFields: []
}
Ok. I think you should debug Sonata\AdminBundle\Admin\AbstractAdmin->generateObjectUrl
, that's the line with the Company object that generates a call with id = null
. Try to find out why it is null
and not 254
.
So, now we're getting somewhere.
In Sonata\ORMAdminBundle\Model\ModelManager->getNormalizedIdentifier
there is
// the entities is not managed
if (!$entity || !$this->getEntityManager($entity)->getUnitOfWork()->isInIdentityMap($entity)) {
return;
}
This will trigger because the entity seems not to be managed at this point.
This will trigger because the entity seems not to be managed at this point.
How do you explain that? Where does it come from?
Well, I have no idea. I'm using simplethings/entity-audit v. 0.9.1, which is the current version. Maybe the entity audit doesn't use the entityManager to get the revisions?
I don't know either :( try to debug the code to see where the Company
object comes from…
The AuditReader of siplaethings/entity-audit is not using the entity manager to load the entity but an sql query. It will use the class metadata of doctrine to get all associations, that's why the data for company is available.
There seems to be some changes in the ModelManager regarding getting the id of an object.
So in fact, for this bundle, maybe the ModelManager should be another class, since the object does not come from the ORM part of doctrine?
Maybe the ModelManager should only try harder to get the id of an entity?
Actually, I've just tried to change
// the entities is not managed
if (!$entity || !$this->getEntityManager($entity)->getUnitOfWork()->isInIdentityMap($entity)) {
return;
}
to
// the entities is not managed
if (!$entity) {
return;
}
This will show the page and generate a proper url, but I have no idea, if this has any unwanted side effects.
Well I don't think you can remove pieces of code like this one. And get away with it just like that :P
However, I think you could override YourAdmin::getNormalizedIdentifier() and if it's a Company, maybe do sth special?
I have found the same code already commented in ModelManager->getIdentifierValues()
.
I have found the same code already commented in ModelManager->getIdentifierValues().
Can you please link to it on GH ?
OK.. so what I've found:
Thomas Rabaix removed the test for performance impacts with: https://github.com/sonata-project/SonataDoctrineORMAdminBundle/commit/6b952402213a3628cd5adea871e6de049c028ea0 https://github.com/sonata-project/SonataDoctrineORMAdminBundle/commit/e52468c35e97872b0b74a5b4417050004d39d9ea
acrobat reintroduced it with:
That might be unintentional.
Oh wow. @rande, @acrobat, can we have your comments on this situation?
@greg0ire sorry for the late answer but I was on vacation. I've re-added the line of code in this PR https://github.com/sonata-project/SonataDoctrineORMAdminBundle/pull/547, because I was using uuid id's in entities and generated the id on construct (so the entity is decoupled from the db/full valid entity before going to the database).
But that way sonata thinks it's an existing entity and the edit screen needs to be show. By enabling the unitofwork check we can check if doctrine manages the entity (== edit) and if not (== create)
Ok so this really is legitimate. @soerenbernstein : Company is not managed by the modelManager in use when reaching this line. If I understand correctly, Company is managed by doctrine orm, but not in this particular case, am I right?
Yes, that is my understanding of the inner workings of the entity audit bundle.
What method exactly do you use to retrieve the company? This one seems to use the entity manager : https://github.com/simplethings/EntityAudit/blob/aac31e0e3ee4d939f8b733ce99ce8d77f7416d6b/src/SimpleThings/EntityAudit/AuditReader.php#L738
I use the default method, there are no overrides. The method is using a DBAL query, not an ORM query. Maybe that is why the unitOfWork doesn't know the entity?
I use the default method, there are no overrides. The method is using a DBAL query, not an ORM query. Maybe that is why the unitOfWork doesn't know the entity?
Yes precisely. Maybe using this method would fix everything. Can you try it?
Sorry, took me a while. Had to do some other things. So, now I've tried and changed the CRUDController:historyViewRevisionAction. Adding the merge command seems to fix the problem. So, how to get this into the trunk?
I don't know if merge is the right method, as it adds a new version of the entity to the entity manager. Maybe you should use the contains
method?
Well, there is no entity for the revision, as far as I understand it, because the audit tables are not managed.
Jokes aside, simply make a PR on the default branch.
as it adds a new version of the entity to the entity manager
Are you sure about that?
Here is what I can read in the code :
First we assume DETACHED, although it can still be NEW but we can avoid an extra db-roundtrip this way. If it is not MANAGED but has an identity, we need to fetch it from the db anyway in order to merge. MANAGED entities are ignored by the merge operation.
If I understand correctly, this will check if the entity exists in the db… but since the audit tables are not manage, it should… fail? Can you check what actually happens @soerenbernstein ?
So, what's actually happen is, that the entity manager finds the entity by id from the managed entity table and changes all properties to the values of the unmanaged object. So basically, your changing you current object to an older state. If the entity manager get a command to persist all changes you would overwrite the current version of the entity. This is not a solution I would like. Also, I just found, that it will not fix all the problems, because I cant get the values of some associations.
Uuuh, not good. Not the problem @acrobat told us about, but still, not good at all…
But now that I re read the ticket, it seems like what is desired here is to link to an unmanaged entity, right? An old version of a Company
object. I can't see how the ORM can help with that at all. IMO, in this particular case, you have to ask the DBAL directly.
I don't want to link to an old version of a company entity. I would be totally happy with the company name as string. Problem is, that it is an association and the (hardcoded) show template used by the controller wants to create a link. Also, this link will be created to to current version, which makes no sense at all in this context. I would be best, if the historyViewRevisionAction would only show the data, no links and no edit ability. Not being able to show a revision of an entity with associations is a regression to SonataAdmin 2.4.x.
To be honest, I'm not familiar at all with this feature, so I can't really answer that. Maybe you should have a look at the git history, find the author of this feature and ping them?
OK, so I have found a solution to fix it for me, although it is a bad hack.I've overridden all showorm
Maybe you should have a look at the git history, find the author of this feature and ping them?
What about that ?
Sorry, had to find a solution that works right now. Yesterday was release day. Also, I really don't want the links in the old revision of the entity unless I get link to the corresponding revision for the association, which is not possible with EntityAudit atm. But I would suggest, that someone from developers test this feature against entity with associations because I think the other association types will have the same problem.
Bug reproduced and confirmed.
If I understand correctly, this will check if the entity exists in the db… but since the audit tables are not manage, it should… fail? Can you check what actually happens
@greg0ire I have this problem with a IpV4
-> Server
relation, both audited, so I don't think so.
@soerenbernstein Concerning https://github.com/sonata-project/SonataAdminBundle/issues/4027#issuecomment-236136644, where did you make this modification?
Did you find a final solution? Or more clues?
This is already fixed by https://github.com/sonata-project/SonataDoctrineORMAdminBundle/pull/631.
Release is coming soon.
Environment
Sonata packages
sonata-project/admin-bundle 3.4.0 The missing Symfony Admin Generator sonata-project/block-bundle 3.0.1 Symfony SonataBlockBundle sonata-project/cache 1.0.7 Cache library sonata-project/core-bundle 3.0.3 Symfony SonataCoreBundle sonata-project/datagrid-bundle 2.2 Symfony SonataDatagridBundle sonata-project/doctrine-extensions 1.0.2 Doctrine2 behavioral extensions sonata-project/doctrine-orm-admin-bundle 3.0.5 Symfony Sonata / Integrate Doctrine ORM into the SonataAdminBundle sonata-project/easy-extends-bundle 2.1.10 Symfony SonataEasyExtendsBundle sonata-project/exporter 1.5.0 Lightweight Exporter library sonata-project/google-authenticator 1.0.2 Library to integrate Google Authenticator into a PHP project sonata-project/intl-bundle 2.2.4 Symfony SonataIntlBundle sonata-project/user-bundle 3.0.1 Symfony SonataUserBundle
Symfony packages
symfony/assetic-bundle v2.8.0 Integrates Assetic into Symfony2 symfony/framework-standard-edition v2.7.15 The "Symfony Standard Edition" distribution symfony/monolog-bundle 2.11.1 Symfony MonologBundle symfony/polyfill-apcu v1.2.0 Symfony polyfill backporting apcu_* functions to lower PHP versions symfony/polyfill-mbstring v1.2.0 Symfony polyfill for the Mbstring extension symfony/polyfill-php54 v1.2.0 Symfony polyfill backporting some PHP 5.4+ features to lower PHP versions symfony/polyfill-php55 v1.2.0 Symfony polyfill backporting some PHP 5.5+ features to lower PHP versions symfony/polyfill-php56 v1.2.0 Symfony polyfill backporting some PHP 5.6+ features to lower PHP versions symfony/polyfill-php70 v1.2.0 Symfony polyfill backporting some PHP 7.0+ features to lower PHP versions symfony/polyfill-util v1.2.0 Symfony utilities for portability of PHP codes symfony/swiftmailer-bundle v2.3.11 Symfony SwiftmailerBundle symfony/symfony v2.7.15 The Symfony PHP framework
PHP version
PHP 5.6.24 (cli) (built: Jul 26 2016 11:42:34) Copyright (c) 1997-2016 The PHP Group Zend Engine v2.6.0, Copyright (c) 1998-2016 Zend Technologies with Zend OPcache v7.0.6-dev, Copyright (c) 1999-2016, by Zend Technologies with Xdebug v2.4.1-dev, Copyright (c) 2002-2016, by Derick Rethans
Subject
The rendering of the show template used for display of a revivision fails, if the entity has a many-to-one-association. I'm in the process of upgrading a running project from SonataAdmin 2.3 to SonataAdmin 3.x and it was working for me in the old version. My CRUDController is extending the default CRUDController and adding some actions as well as extending the create action. The edit action ist not overwritten.
Steps to reproduce
View a revision of an entity with a many-to-one association.
Expected results
The display of the entity revision.
Actual results
An exception has been thrown during the rendering of a template ("Parameter "id" for route "admin_redlink_cas_company_edit" must match "[^/]++" ("" given) to generate a corresponding URL.") in SonataDoctrineORMAdminBundle:CRUD:show_orm_many_to_one.html.twig at line 19.
[1] Twig_Error_Runtime: An exception has been thrown during the rendering of a template ("Parameter "id" for route "admin_redlink_cas_company_edit" must match "[^/]++" ("" given) to generate a corresponding URL.") in "SonataDoctrineORMAdminBundle:CRUD:show_orm_many_to_one.html.twig" at line 19. at n/a in /var/www/users/sob/cas-dev-sa3/vendor/twig/twig/lib/Twig/Template.php line 182
[2] Symfony\Component\Routing\Exception\InvalidParameterException: Parameter "id" for route "admin_redlink_cas_company_edit" must match "[^/]++" ("" given) to generate a corresponding URL. at n/a in /var/www/users/sob/cas-dev-sa3/vendor/symfony/symfony/src/Symfony/Component/Routing/Generator/UrlGenerator.php line 163