Added missing events. #245

Merged
merged 4 commits into from Feb 19, 2013

Projects

None yet

4 participants

Contributor

Added the 3 remaining events that are implemented in the ORM but not the ODM.

  • preFlush
  • loadClassMetadata
  • postFlush

Works in theory, havn't tested in a concrete implementation yet.

@dantleech dantleech Added support for postFlush event
- This event allows ->flush() to be called from its listeners.
cc941c8
Member
dbu commented Feb 18, 2013

thanks. whyever we did not have that in the first place. probably was added to ORM after we started the PHPCR-ODM, or it was/is missing in MongoDB-ODM from which the PHPCR-ODM initially started...

i wonder why we need a custom event arguments class for this, but it seems consistent with the ORM.

so all said, good to merge for me. i will wait another day for additional feedback. could you please also update the doctrine doc about this?

Contributor

Yeah, this event was added relatively recently to the ORM. Will make a
PR for the docs.

On Mon, Feb 18, 2013 at 12:17:42AM -0800, David Buchmann wrote:

thanks. whyever we did not have that in the first place. probably was added to
ORM after we started the PHPCR-ODM, or it was/is missing in MongoDB-ODM from
which the PHPCR-ODM initially started...

i wonder why we need a custom event arguments class for this, but it seems
consistent with the ORM.

so all said, good to merge for me. i will wait another day for additional
feedback. could you please also update the doctrine doc about this?


Reply to this email directly or view it on GitHub.

Contributor

Have also added support for loadClassMetadata and preFlush which completes the feature set as onFlush and onClear were already implemented but not documented.

Travis is failing with some translation problems, I'm guessing that problem exists already?

@dantleech dantleech referenced this pull request in doctrine/phpcr-odm-documentation Feb 18, 2013
Merged

Updated events page to include all events #18

Member
dbu commented Feb 18, 2013

yeah the tests seem to get fixed in #237 - i think this was a PR to demo a bug that was merged, and the fixing takes some time to get things right.

Contributor

Hi,

I had not seen this PR when I make the PR #246.

So you could add preMove and postMove events too or merge with my PR ?

Thanks

Member
dbu commented Feb 19, 2013

hi @laupiFrpar, thanks for that PR. i think it makes sense to keep both PR separate for now. i added some comments in your PR. if it turns out we need to do some common refactoring, it could make sense to merge the 2 PR. but probably we merge them separately and do whatever cleanup refactoring is needed in a new PR.

Member
dbu commented Feb 19, 2013

@lsmith77 can you give feedback on this PR, i think its ok to merge. though i am not happy about the seemingly redundant event argument classes. not sure if there is a good reason to have them in the ORM.

@lsmith77 lsmith77 merged commit 942062e into doctrine:master Feb 19, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment