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: Jump links #9131

Merged
merged 2 commits into from
Oct 25, 2021

Conversation

ThomasLandauer
Copy link
Contributor

Well, the answer to what I said at #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

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**
@SenseException
Copy link
Member

  1. Sure, links might help to get more infos on an event
  2. Sounds reasonable :-)
  3. Is this supposed to be a new column for the table? One that shows if it's meant to change an entity?

@ThomasLandauer
Copy link
Contributor Author

I'm done with the links, please merge.

Remarks/questions:
1: onClear doesn't have a section. So I'd create one, just with the short info from the list https://www.doctrine-project.org/projects/doctrine-orm/en/2.10/reference/events.html#lifecycle-events
2: onClassMetadataNotFound is dispatched when loading fails (right?), so I'll just add a short note to https://www.doctrine-project.org/projects/doctrine-orm/en/2.10/reference/events.html#load-classmetadata-event (i.e. don't create a dedicated section for this)
3: I'll do the changing of the order in a separate PR, otherwise this would be impossible to review ;-)
4: "Can change current entity": Yes, that would be a new column in the table. Answering the question: If I modify the current entity there (e.g. $entity->setFoo(...)), will this have an effect, or is it already too late?

@derrabus derrabus added this to the 2.10.3 milestone Oct 21, 2021
@SenseException
Copy link
Member

onClear doesn't have a section. So I'd create one, just with the short info from the list https://www.doctrine-project.org/projects/doctrine-orm/en/2.10/reference/events.html#lifecycle-events

onClassMetadataNotFound is dispatched when loading fails (right?), so I'll just add a short note to https://www.doctrine-project.org/projects/doctrine-orm/en/2.10/reference/events.html#load-classmetadata-event (i.e. don't create a dedicated section for this)

Did you mean in this PR? Should I wait with further review or will you introduce it in another PR?

@ThomasLandauer
Copy link
Contributor Author

Did you mean in this PR?

No! Please review and merge this as-is, then I'll continue in a new one.

@SenseException SenseException merged commit 3622381 into doctrine:2.10.x Oct 25, 2021
@SenseException
Copy link
Member

Thanks @ThomasLandauer 🌻

@ThomasLandauer ThomasLandauer deleted the patch-1 branch October 26, 2021 11:41
ThomasLandauer added a commit to ThomasLandauer/orm that referenced this pull request Oct 26, 2021
SenseException pushed a commit that referenced this pull request Oct 28, 2021
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.

3 participants