Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.3k
Encourage use of constants#10308
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Function should be static and we should also encourage the use of constants
trsteel88 commentedSep 10, 2018
Sorry, it shouldn't be static as that method comes from Doctrine and differs to Symfony's event subscriber. Updated that and just kept the constants. |
javiereguiluz commentedSep 10, 2018
@trsteel88 thanks for this contribution! However, I think the new code is different than the existing code. Is this a bug fix, an improvement or both? We'll need help from Doctrine experts to verify this proposal. Thanks! |
ndench left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@javiereguiluz this doesn't change the existing code. You can see from thedoctrine source that event constants are the same as the original strings.
Also thedoctrine documentation recommends using the event constants:
<?phpuse Doctrine\ORM\Events;echo Events::preUpdate;| 'postPersist', | ||
| 'postUpdate', | ||
| \Doctrine\ORM\Events::postPersist =>'postPersist', | ||
| \Doctrine\ORM\Events::postUpdate =>'postUpdate', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we should importDoctrine\ORM\Events and useEvents::postPersist andEvents::postUpdate here instead.
xabbuh commentedNov 15, 2018
Thank you for this change@trsteel88. I have merged your changes into the |
* 2.8: [symfony#10308] add use statement and fix array Encourage use of constants delete var_dump() functions in examples
* 3.4: [symfony#10308] add use statement and fix array Encourage use of constants Update code-splitting.rst delete var_dump() functions in examples Update image to reflect given state machine example configuration
* 4.1: [symfony#10308] add use statement and fix array Encourage use of constants Update code-splitting.rst delete var_dump() functions in examples Update image to reflect given state machine example configuration
trsteel88 commentedNov 15, 2018
No worries. Sorry I fell off the face of the earth. Been a bit hectic. |
Function should be static and we should also encourage the use of constants