Skip to content

Fix wrong place of parent constructor call #9863

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
Closed
wants to merge 1 commit into from
Closed

Fix wrong place of parent constructor call #9863

wants to merge 1 commit into from

Conversation

Nek-
Copy link
Contributor

@Nek- Nek- commented Jun 1, 2018

Calling parent constructor after doing stuff in the overrided constructor is a bad habit. The documentation should encourage good practices.

@Nek-
Fix wrong place of parent constructor call
7b3b9fc 
Calling parent constructor after doing stuff in the overrided
constructor is a bad habit.
@javiereguiluz javiereguiluz added this to the 2.8 milestone Jun 1, 2018
javiereguiluz
javiereguiluz approved these changes Jun 1, 2018
View reviewed changes
Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

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

I agree this is a better practice. Thanks!

@yceruto
Copy link
Member

yceruto commented Jun 1, 2018 •
edited
Loading

In general Yes! but I guess it was on purpose for this case, because console commands call to configure() method at the end of the constructor, so any argument used there, is already setted, avoiding issues like this symfony/symfony#27224

@Nek-
Copy link
Contributor Author

Nek- commented Jun 5, 2018

configure method should not use the given parameters to the constructor. So I guess my change is still relevant.

@javiereguiluz javiereguiluz mentioned this pull request Jun 27, 2018
Merged
@javiereguiluz
Copy link
Member

I've reviewed the information and the PR linked by Yonel and I think we must explain this better in the docs. That's why I'm closing this PR in favor of #9969.

@Nek- thanks for reporting this problem and helping us improve Symfony!

javiereguiluz added a commit that referenced this pull request Jun 27, 2018
@javiereguiluz
minor #9969 Explained some issues about command constructors and conf…
05fbb9c 
…igure (javiereguiluz)

This PR was merged into the 3.4 branch.

Discussion
----------

Explained some issues about command constructors and configure

This fixes #9863 differently.

Commits
-------

27beba5 Explained some issues about command constructors and configure
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javiereguiluz javiereguiluz javiereguiluz approved these changes

Assignees
No one assigned
Labels
Status: Reviewed
Projects
None yet
Milestone
2.8
Development

Successfully merging this pull request may close these issues.
4 participants
@Nek- @yceruto @javiereguiluz @carsonbot