Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Encourage use of constants #10308

wants to merge 2 commits into from

Conversation

trsteel88
Copy link
Contributor

Function should be static and we should also encourage the use of constants

Function should be static and we should also encourage the use of constants
@trsteel88
Copy link
Contributor Author

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.

@trsteel88 trsteel88 changed the title Method should be static. Encourage use of const Encourage use of constants Sep 10, 2018
@javiereguiluz
Copy link
Member

@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!

Copy link
Contributor

@ndench ndench left a 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 the doctrine source that event constants are the same as the original strings.

Also the doctrine documentation recommends using the event constants:

<?php
use Doctrine\ORM\Events;
echo Events::preUpdate;

'postPersist',
'postUpdate',
\Doctrine\ORM\Events::postPersist => 'postPersist',
\Doctrine\ORM\Events::postUpdate => 'postUpdate',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should import Doctrine\ORM\Events and use Events::postPersist and Events::postUpdate here instead.

@xabbuh xabbuh added this to the 2.8 milestone Nov 15, 2018
xabbuh added a commit that referenced this pull request Nov 15, 2018
This PR was submitted for the 4.1 branch but it was squashed and merged into the 2.8 branch instead (closes #10308).

Discussion
----------

Encourage use of constants

Function should be static and we should also encourage the use of constants

Commits
-------

10422dc Encourage use of constants
xabbuh added a commit that referenced this pull request Nov 15, 2018
@xabbuh
Copy link
Member

xabbuh commented Nov 15, 2018

Thank you for this change @trsteel88. I have merged your changes into the 2.8 branch (from where it will be merged up to all maintained branches) and made some small tweaks in 88939de.

@xabbuh xabbuh closed this Nov 15, 2018
javiereguiluz added a commit to javiereguiluz/symfony-docs that referenced this pull request Nov 15, 2018
* 2.8:
  [symfony#10308] add use statement and fix array
  Encourage use of constants
  delete var_dump() functions in examples
javiereguiluz added a commit to javiereguiluz/symfony-docs that referenced this pull request Nov 15, 2018
* 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
javiereguiluz added a commit to javiereguiluz/symfony-docs that referenced this pull request Nov 15, 2018
* 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 trsteel88 deleted the patch-1 branch November 15, 2018 23:16
@trsteel88
Copy link
Contributor Author

No worries. Sorry I fell off the face of the earth. Been a bit hectic.

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.

5 participants