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.

Sign up for GitHub

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

Jump to bottom

[WIP] Refactoring of event listeners(closes #8, #89, #123, #297) #617

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

[WIP] Refactoring of event listeners(closes #8, #89, #123, #297) #617

wants to merge 2 commits into from

Conversation

Koc
Copy link
Contributor

@Koc Koc commented Sep 1, 2016 •
edited
Loading

Code is not tested yet, WIP.

  1. do not call recompute changeset because changesets not yet computed when preFlush triggers
  2. fixes The file is not updated if there are not other changes in the entity #8, How modify a image ? #89, The file upload is not picked up, if the file is the only field that has changed #123, Upload is not performed if there is no change in the entity #297
  3. register only one event listener per db driver instead of registering one event listener for each mapping
  4. reduce calls for reading metadata

Looks like AdapterInterface::recomputeChangeSet is not called anymore.

@Koc Koc changed the title [WIP] Use preFlush event introduced in Doctrine 2.2 (closes #89) [WIP] Use preFlush event introduced in Doctrine 2.2 (closes #8, #89, #123) Sep 2, 2016
@Koc Koc force-pushed the event-listeners branch 2 times, most recently from d91613a to 4f9daa1 Compare September 3, 2016 00:13
@Koc Koc changed the title [WIP] Use preFlush event introduced in Doctrine 2.2 (closes #8, #89, #123) [WIP] Refactoring of event listeners(closes #8, #89, #123, #619) Sep 3, 2016
@Koc Koc changed the title [WIP] Refactoring of event listeners(closes #8, #89, #123, #619) [WIP] Refactoring of event listeners(closes #8, #89, #123) Sep 3, 2016
@Koc Koc mentioned this pull request Sep 3, 2016
Closed
@Koc Koc force-pushed the event-listeners branch 2 times, most recently from f804e86 to accb50f Compare September 14, 2016 15:18
@Koc Koc changed the title [WIP] Refactoring of event listeners(closes #8, #89, #123) [WIP] Refactoring of event listeners(closes #8, #89, #123, #297) Sep 14, 2016
@Koc Koc mentioned this pull request Nov 13, 2016
Closed
stof
stof reviewed Nov 16, 2016
View reviewed changes
EventListener/Doctrine/UploadListener.php Outdated
'prePersist',
'preUpdate',
);
return array(Events::preFlush);
Copy link
Contributor

Choose a reason for hiding this comment

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

this breaks for people using the ODM rather than the ORM as they won't have the constant defined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What you suggest: use strings or create different classes wich rely on different class contsants?

Copy link
Contributor

Choose a reason for hiding this comment

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

use strings

EventListener/Doctrine/UploadListener.php Outdated
*/
public function prePersist(EventArgs $event)
public function preFlush(PreFlushEventArgs $event)
Copy link
Contributor

Choose a reason for hiding this comment

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

this typehint will break as well for other Doctrine projects

Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove typehint or create different class for ODM?

Copy link
Contributor

Choose a reason for hiding this comment

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

typehint using only the classes from Doctrine Common (and add instanceof checks internally if they have not settled on a common API for the preflush event but this should also be reported to Doctrine as events have been more or less standardized in doctrine/common, but Doctrine\Common\EventArgs should extend \Doctrine\Common\Persistence\Event\ManagerEventArgs in future versions to be consistent with other events)

preflush is not properly standardized among Doctrine projects yet (check the case of the ODM fully then btw, as this event might cause you some issues)

@Koc
Copy link
Contributor Author

Koc commented Nov 16, 2016

@stof what can you say about this PR in general? And I have some problem with Propel integration. Not sure this approach will work correctly

stof
stof reviewed Nov 17, 2016
View reviewed changes
EventListener/Doctrine/CleanListener.php Outdated
{
$object = $this->adapter->getObjectFromArgs($event);
$em = $event->getEntityManager();
$identityMap = $em->getUnitOfWork()->getIdentityMap();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an internal Doctrine API AFAIK. Relying on it may not be the best idea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How can I got changed entities in other way? PreFlush event not recieve entities

EventListener/Doctrine/InjectListener.php Outdated
@@ -2,42 +2,40 @@

namespace Vich\UploaderBundle\EventListener\Doctrine;

use Doctrine\Common\EventArgs;
use Doctrine\ORM\Event\LifecycleEventArgs;
Copy link
Contributor

Choose a reason for hiding this comment

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

you should use the doctrine/common class

@garak
Copy link
Collaborator

garak commented Dec 18, 2016

@Koc can you solve conflict? And maybe adding some tests?

@Koc
Copy link
Contributor Author

Koc commented Dec 18, 2016

@garak yes, will do it later after fiinish my other PRs. Also I should apply changes requested by @stof . This PR is much harder than other.

@Koc Koc mentioned this pull request Dec 18, 2016
Closed
@Koc Koc mentioned this pull request May 10, 2017
Open
10 tasks
@Koc
Copy link
Contributor Author

Koc commented May 25, 2017

branch restarted and rebased. I'm afraid that this PR breaks BC. But we can start working on 2.0 where BC-breaks are acceptable.

@garak garak marked this pull request as draft July 24, 2022 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The file is not updated if there are not other changes in the entity
3 participants
@Koc @garak @stof