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

Add documentation for AsEventListener attribute #1583

Merged
merged 2 commits into from
Dec 30, 2022
Merged

Add documentation for AsEventListener attribute #1583

merged 2 commits into from
Dec 30, 2022

Conversation

jderusse
Copy link
Contributor

@jderusse jderusse commented Nov 21, 2022 •
edited
Loading

This PR adds documentation for AsEventListener attribute introduced in #1561

@jderusse jderusse force-pushed the att-event branch 3 times, most recently from 7d78785 to 9e5b758 Compare November 21, 2022 22:36
@ostrolucky
Copy link
Member

can you update doc/ too?

@jderusse jderusse mentioned this pull request Nov 27, 2022
Merged
@jderusse
Copy link
Contributor Author

Documentation added here (DoctrineBundle) and also in symfony/symfony-docs symfony/symfony-docs#17472

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Nov 27, 2022 •
edited
Loading

What about naming this one AsOrmListener? Symfony already has an AsEvenListener attribute and that could be confusing.

@ostrolucky ostrolucky added this to the 2.8.0 milestone Nov 27, 2022
@ostrolucky
Copy link
Member

This confusion just reflects how it is in DI tags, so it's fine I think. If we name this AsOrmListener, people would then wonder why tag isn't doctrine.orm.event_listener

@dmaicher
Copy link
Contributor

dmaicher commented Dec 7, 2022

And actually doctrine.event_listener is not purely an ORM concept, or? I think this will also work for DBAL events.

dmaicher
dmaicher reviewed Dec 7, 2022
View reviewed changes
Copy link
Contributor

@dmaicher dmaicher left a comment

Choose a reason for hiding this comment

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

👍

Regarding the name: I agree its slightly confusing with Symfony's AsEventListener. Maybe AsDoctrineEventListener could be an alternative? 🤷‍♂️

Attribute/AsEventListener.php Outdated Show resolved Hide resolved
Resources/doc/event-listeners.rst Outdated Show resolved Hide resolved
@dmaicher
Copy link
Contributor

dmaicher commented Dec 7, 2022

@jderusse can you change the base to 2.8.x? Since this is a new feature we should target 2.8

@jderusse
Copy link
Contributor Author

jderusse commented Dec 7, 2022

ok... this is embarrassing now... While rebasing on 2.8.x I just realized this is already implemented in #1561....

I keep this PR open for documentation purposes

@jderusse jderusse changed the base branch from 2.7.x to 2.8.x December 7, 2022 20:08
@jderusse jderusse changed the title Add AsEventListener attribute Add documentation for AsEventListener attribute Dec 7, 2022
dmaicher
dmaicher reviewed Dec 8, 2022
View reviewed changes
Copy link
Contributor

@dmaicher dmaicher left a comment

Choose a reason for hiding this comment

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

Oh wow I also totally missed that this was already done on 2.8 🙈

Some tiny remarks left from my side 👍

Resources/doc/event-listeners.rst Outdated Show resolved Hide resolved
Resources/doc/event-listeners.rst Outdated Show resolved Hide resolved
Resources/doc/event-listeners.rst Outdated Show resolved Hide resolved
@nicolas-grekas nicolas-grekas mentioned this pull request Dec 8, 2022
Merged
@nicolas-grekas
Copy link
Member

I'd like we rename the attribute, here is my proposal: #1592

@ostrolucky ostrolucky closed this Dec 8, 2022
@ostrolucky ostrolucky reopened this Dec 8, 2022
@dmaicher
Copy link
Contributor

dmaicher commented Dec 8, 2022

@jderusse the attribute was renamed to AsDoctrineListener now. Are you up for adjusting the documentation?

@derrabus derrabus modified the milestones: 2.8.0, 2.8.1 Dec 29, 2022
@ostrolucky
Copy link
Member

I've updated it. Any other feedback?

@ostrolucky ostrolucky merged commit 1c7a4f3 into doctrine:2.8.x Dec 30, 2022
@github-actions github-actions bot mentioned this pull request Jan 6, 2023
Merged
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Mar 3, 2023
@javiereguiluz
minor #17472 [Doctrine] Add documentation about Doctrine `AsEventList…
c51f145 
…ener` (jderusse)

This PR was merged into the 6.2 branch.

Discussion
----------

[Doctrine] Add documentation about Doctrine `AsEventListener`

Related to the **NOT YET MERGED** PR doctrine/DoctrineBundle#1583

Commits
-------

e97a574 Add documentation about doctrine's AsEventListener
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmaicher dmaicher dmaicher approved these changes

@ostrolucky ostrolucky ostrolucky approved these changes

Assignees
No one assigned
Labels
Documentation Feature
Projects
None yet
Milestone
2.8.1
Development

Successfully merging this pull request may close these issues.
6 participants
@jderusse @ostrolucky @nicolas-grekas @dmaicher @SenseException @derrabus