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

Made entity listener documentation less ambiguous #910

Merged
merged 1 commit into from Feb 5, 2019
Merged

Made entity listener documentation less ambiguous #910

merged 1 commit into from Feb 5, 2019

Conversation

romaricdrigon
Copy link
Contributor

Hello,

Entity listener documentation was giving different syntaxes, without explaining was was required exactly and for which versions to use those. It was hard to understand the entity annotation is required if you use the "short" service definition, or that you can use the "long" comprehensive service definition in place.
It was misleading for new users, and even for senior ones, I got it wrong today.

This change proposal hopes to make it clearer, but explaining the 2 different syntaxes one after the other.

@romaricdrigon
Copy link
Contributor Author

Thank you for the proofreading @greg0ire
I will squash all commits together.

@SenseException
Copy link
Member

In case #906 gets merged before this PR, a rebase will be needed.

Resources/doc/entity-listeners.rst Outdated Show resolved Hide resolved
Resources/doc/entity-listeners.rst Outdated Show resolved Hide resolved
@romaricdrigon
Copy link
Contributor Author

Updated & commits squashed. @SenseException I hope those 2 PR can get merged soon, the documentation is really opaque right now and I'm sure it is impacting a lot of readers. No problems for rebasing/updating my PR if #906 is merged before, but maybe this PR would supersede it?

Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @romaricdrigon! Could I ask you to rebase the PR against the 1.10 branch so we can fix the docs for the currently stable version? Thanks!

@romaricdrigon romaricdrigon changed the base branch from master to 1.10.x February 1, 2019 10:29
@romaricdrigon
Copy link
Contributor Author

Sure @alcaeus , here you are :)

Resources/doc/entity-listeners.rst Outdated Show resolved Hide resolved
@alcaeus
Copy link
Member

alcaeus commented Feb 3, 2019

@romaricdrigon I merged #906 not realising that there is a lot more documentation that was missing. Could you please rebase the PR one last time and fix the issue I highlighted above? Other than that, this is good to go.

@romaricdrigon
Copy link
Contributor Author

Hello @alcaeus,
I rebased and updated my PR. Reading again #906, I realized having 2 configuration example (one without entity manager option, one with) could be misleading. So I simplified it using comments, I hope it makes more sense.

Entity listener documentation was giving different syntaxes, without explaining was was required exactly and for which versions to use those. This change clears it up.
@alcaeus alcaeus requested a review from greg0ire February 5, 2019 18:44
Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

Looks good from my end, thanks @romaricdrigon!

I've made some more changes to cleanly wrap at 80 characters and removed some trailing spaces.

@alcaeus alcaeus self-assigned this Feb 5, 2019
@alcaeus alcaeus added this to the 1.10.2 milestone Feb 5, 2019
@alcaeus alcaeus merged commit e54bf58 into doctrine:1.10.x Feb 5, 2019
@romaricdrigon romaricdrigon deleted the patch-1 branch February 6, 2019 09:03
@romaricdrigon
Copy link
Contributor Author

The symfony.com documentation (https://symfony.com/doc/master/bundles/DoctrineBundle/entity-listeners.html) was never updated, do you know why? Is it based on another branch (master?)?

@alcaeus
Copy link
Member

alcaeus commented Mar 19, 2019

I can take a look at that next week, will be out for a few days for personal reasons.

@romaricdrigon
Copy link
Contributor Author

Sure, thank you

@romaricdrigon
Copy link
Contributor Author

I saw the Symfony website was updated, thanks :)

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