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

Overview table for events #9039

Merged
merged 5 commits into from Oct 14, 2021
Merged

Overview table for events #9039

merged 5 commits into from Oct 14, 2021

Conversation

ThomasLandauer
Copy link
Contributor

@ThomasLandauer ThomasLandauer commented Sep 27, 2021

Better late than never - finally delivering what I announced at #8435 (comment) :-)

@SenseException Could you please fill in the two question marks? Or just delete those two rows if the events are not important ;-)

Then please merge this as-is. The plan for this table is to replace the list at https://www.doctrine-project.org/projects/doctrine-orm/en/2.9/reference/events.html#lifecycle-events completely. But I'd like to proceed column by column. And before doing more stupid - character counting ;-) I'd like to see if this does come out nicely on the web page...

Next step would be to link the events (first column) to their respective headers/anchors further down the page, e.g.:

[`prePersist`](#prepersist)

@ThomasLandauer ThomasLandauer marked this pull request as ready for review September 27, 2021 22:23
@SenseException
Copy link
Member

I have to lookup the missing pieces myself. I'll come back to it.

@ThomasLandauer
Copy link
Contributor Author

Sure. Can I ask you something in the meantime :-)

At #8435 (comment) you said:

I just realized that the events at the top are representing the Lifecycle Events while the bottom ones are Event Listeners.

And the list at https://www.doctrine-project.org/projects/doctrine-orm/en/2.9/reference/events.html#lifecycle-events says for several events: "This event is not a lifecycle callback."
What does this mean?? If those two ClassMetadata events (and maybe the other non-lifecycle-callbacks) are some special case or rarely used, it's maybe better to exclude them from this table and explain them separately (extra table, list or whatever...)?

Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

"This event is not a lifecycle callback." What does this mean?

I suppose this is referring to https://www.doctrine-project.org/projects/doctrine-orm/en/2.9/reference/events.html#lifecycle-callbacks where lifecycle callbacks are used in the entity class. Maybe a third "is lifecycle callback" column can mark those who can be used as a lifecycle callback.

If those two ClassMetadata events (and maybe the other non-lifecycle-callbacks) are some special case or rarely used, it's maybe better to exclude them from this table and explain them separately (extra table, list or whatever...)?

I never had a use case where I had to use these ClassMetadata events, but I wouldn't call them special. The table is an overview and doesn't get too much into detail, so I think it's fine to keep those events in there. If there's a need to tell more detailed information about them, then those can be additionally appended as text.

+-----------------------------+-----------------------+
| ``postLoad`` | Loading from database |
+-----------------------------+-----------------------+
| ``loadClassMetadata`` | ? |
Copy link
Member

Choose a reason for hiding this comment

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

This is hard to point out. It is triggered when metadata is loaded, which happens in reading methods of the entity manager like find(), getReference() and getPartialReference() when it is loaded for the first time. Of course it is also triggered when $metadataFactory->getMetadataFor() is called directly.

If this table is focused on the entity manager, maybe those three $em methods can be added with the hint of an initial load.

It is also triggered on $em->clear('Foo'), but the argument for clear() is deprecated and will be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe those three $em methods can be added with the hint of an initial load.

Does "initial" mean (a) for the first time in the current request, or (b) for the first time ever (i.e. until the database structure is changed)? If (a), then there's almost no (?) difference to what I wrote for postLoad (="Loading from database"), since you probably don't load the same entity 20 times in the same request...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

docs/en/reference/events.rst Outdated Show resolved Hide resolved
@derrabus derrabus added this to the 2.9.6 milestone Oct 2, 2021
@greg0ire greg0ire modified the milestones: 2.9.6, 2.10.0 Oct 3, 2021
@greg0ire
Copy link
Member

greg0ire commented Oct 3, 2021

Unassigning from milestone 2.9.6 since I'm trying to release it.

@greg0ire greg0ire removed this from the 2.10.0 milestone Oct 3, 2021
@ThomasLandauer
Copy link
Contributor Author

Maybe a third "is lifecycle callback" column can mark those who can be used as a lifecycle callback.

Yeah sure! But what I meant is: If some are "lifecycle callbacks", what are the others then? Can you not use them in an entity??

@derrabus derrabus added this to the 2.10.1 milestone Oct 3, 2021
@derrabus derrabus changed the base branch from 2.9.x to 2.10.x October 3, 2021 16:38
@SenseException
Copy link
Member

You can't use them as annotation in an entity, if you mean that. That's right. ORM is using an EventManager where eventlisteners can be registered.

@ThomasLandauer
Copy link
Contributor Author

OK, I'm starting to understand ;-) So the rule is:

  • All events can be registered through the EventManager like this?:
    $entityManager->getEventManager()->addEventListener(array(Events::preUpdate), new MyEventListener());
  • "lifecycle callbacks" can also be registered with an annotation in the entity

Right?

@derrabus derrabus modified the milestones: 2.10.1, 2.10.2 Oct 5, 2021
@SenseException
Copy link
Member

Right, this should summing it up.

ThomasLandauer added a commit to ThomasLandauer/orm that referenced this pull request Oct 5, 2021
@ThomasLandauer
Copy link
Contributor Author

@SenseException Please merge this as-is - I'd like to see how it looks, and what can already be deleted from the list at https://www.doctrine-project.org/projects/doctrine-orm/en/2.9/reference/events.html#lifecycle-events

About loadClassMetadata (see our unresolved conversation above): There's no way to get all this into the table ;-) So I'd say it should be added to https://www.doctrine-project.org/projects/doctrine-orm/en/2.9/reference/events.html#load-classmetadata-event If I should write it, I'd have to ask you some more questions - so it's probably faster if you write it... :-)

Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

I guess this should work. :-)

I'd like to have 2 more eyes to look over the table content.

@derrabus derrabus merged commit e313d01 into doctrine:2.10.x Oct 14, 2021
@SenseException
Copy link
Member

Thank you @ThomasLandauer

@ThomasLandauer ThomasLandauer deleted the patch-5 branch October 17, 2021 12:26
ThomasLandauer added a commit to ThomasLandauer/orm that referenced this pull request Oct 17, 2021
Well, the answer to what I said at doctrine#9039 (comment)

> I'd like to see if this does come out nicely on the web page...

... is clearly NO ;-)

Anyway, let's continue and care about the layout later...

Questions:

1:
Is it OK to link them like so? => Then I'll do the others.

2:
The order of the events differs between the table (=same as list) and the chapters further down. I think the chapters make more sense (starting with `prePersist`), so I'm going to change the table.

3:
Suggestion for the next table column: **Can change current entity**
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants