Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add lead to the log only after persisted to avoid cascade persist error #5804

Merged
merged 4 commits into from Mar 22, 2018

Conversation

escopecz
Copy link
Sponsor Member

@escopecz escopecz commented Mar 13, 2018

Q A
Bug fix? Y
New feature? N
Automated tests included? Y
Related user documentation PR URL /
Related developer documentation PR URL /
Issues addressed (#s or URLs) /
BC breaks? N
Deprecations? N

Description:

I'm trying to fix this error:

Doctrine\ORM\ORMInvalidArgumentException: A new entity was found through the relationship 'Mautic\LeadBundle\Entity\LeadEventLog#lead' that was not configured to cascade persist operations for entity: Mautic\LeadBundle\Entity\Lead with ID #. To solve this issue: Either explicitly call EntityManager#persist() on this unknown entity or configure cascade persist  this association in the mapping for example @ManyToOne(..,cascade={"persist"}). in vendor/doctrine/orm/lib/Doctrine/ORM/ORMInvalidArgumentException.php:92
Stack trace:
#0 vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php(852): Doctrine\ORM\ORMInvalidArgumentException::newEntityFoundThroughRelationship(Array, Object(Mautic\LeadBundle\Entity\Lead))
#1 vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php(740): Doctrine\ORM\UnitOfWork->computeAssociationChanges(Array, Object(Mautic\LeadBundle\Entity\Lead))
#2 vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php(452): Doctrine\ORM\UnitOfWork->computeChangeSet(Object(Doctrine\ORM\Mapping\ClassMetadata), Object(Mautic\LeadBundle\Entity\LeadEventLog))
#3 vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php(485): Doctrine\ORM\UnitOfWork->computeScheduleInsertsChangeSets()
#4 vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php(342): Doctrine\ORM\UnitOfWork->computeSingleEntityChangeSet(Object(Mautic\LeadBundle\Entity\LeadEventLog))
#5 vendor/doctrine/orm/lib/Doctrine/ORM/EntityManager.php(356): Doctrine\ORM\UnitOfWork->commit(Object(Mautic\LeadBundle\Entity\LeadEventLog))
#6 app/bundles/CoreBundle/Entity/CommonRepository.php(782): Doctrine\ORM\EntityManager->flush(Object(Mautic\LeadBundle\Entity\LeadEventLog))
#7 app/bundles/LeadBundle/Model/ImportModel.php(510): Mautic\CoreBundle\Entity\CommonRepository->saveEntity(Object(Mautic\LeadBundle\Entity\LeadEventLog))
#8 app/bundles/LeadBundle/Model/ImportModel.php(393): Mautic\LeadBundle\Model\ImportModel->logImportRowError(Object(Mautic\LeadBundle\Entity\LeadEventLog), 'Lead limit exce...')
#9 app/bundles/LeadBundle/Controller/ImportController.php(240): Mautic\LeadBundle\Model\ImportModel->process(Object(Mautic\LeadBundle\Entity\Import), Object(Mautic\LeadBundle\Helper\Progress))
#10 app/bundles/CoreBundle/Controller/CommonController.php(475): Mautic\LeadBundle\Controller\ImportController->newAction(0, '')
#11 [internal function]: Mautic\CoreBundle\Controller\CommonController->executeAction('new', 0, 0, '')

The problem is that I don't quite know how to replicate it. But from the code I think it happens like this:

  1. A contact is being imported.
  2. The LeadEventLog is instantiated.
  3. The Lead is associated to the LeadEventLog
  4. The Lead is being saved but failed by throwing an exception.
  5. The exception is caught and the LeadEventLog is saved but it fails because it has a Lead entity associated from 3.

So my solution is to associate Lead with LeadEventLog only if the Lead save ends with a success.

Steps to reproduce the bug:

  1. Dunno

Steps to test this PR:

  1. Run $ bin/phpunit -d memory_limit=2048M --bootstrap vendor/autoload.php --configuration app/phpunit.xml.dist --filter LeadModelTest
  2. Try to import a CSV and check that it was a success.

@mautibot mautibot added the code-review-needed PR's that require a code review before merging label Mar 13, 2018
@escopecz escopecz added bug Issues or PR's relating to bugs ready-to-test PR's that are ready to test and removed review labels Mar 13, 2018
@escopecz escopecz added this to the 2.13.0 milestone Mar 13, 2018
@@ -1920,6 +1919,10 @@ public function import($fields, $data, $owner = null, $list = null, $tags = null
if ($company !== null) {
$this->companyModel->addLeadToCompany($company, $lead);
}

if ($eventLog) {
$lead->addEventLog($eventLog);
Copy link
Contributor

@Maxell92 Maxell92 Mar 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this change will be saved to database? No flush is called here.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. Because of https://github.com/mautic/mautic/pull/5804/files#diff-9784b7fe0e4e02b9768b1efa4c50fe25R393 and https://github.com/mautic/mautic/pull/5804/files#diff-9784b7fe0e4e02b9768b1efa4c50fe25R396.

I also tested it and it stores all rows to the log table. Before the last commit it stored only errors.

@Maxell92 Maxell92 self-assigned this Mar 21, 2018
Copy link
Contributor

@Maxell92 Maxell92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me

@mautibot mautibot removed the code-review-needed PR's that require a code review before merging label Mar 21, 2018
@Maxell92 Maxell92 added the pending-test-confirmation PR's that require one test before they can be merged label Mar 21, 2018
@javjim
Copy link

javjim commented Mar 22, 2018

tests pass

@javjim javjim added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed pending-test-confirmation PR's that require one test before they can be merged ready-to-test PR's that are ready to test labels Mar 22, 2018
@escopecz escopecz removed the ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged label Mar 22, 2018
@escopecz escopecz merged commit 0f1731e into mautic:staging Mar 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants