Skip to content

Remove Tag::setName #1357

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

Merged
merged 1 commit into from
Oct 28, 2022
Merged

Remove Tag::setName #1357

merged 1 commit into from
Oct 28, 2022

Conversation

chr-hertel
Copy link
Contributor

In this case we can move it to constructor

@TheDevick
Copy link

Can we use a readonly proprierty in the Entity and keep the setName function?

Why?

Just because sometimes it makes sense to call the constructor with no parameters - new Tag(); and then call the setName() function, so making the code more readable in some cases

@chr-hertel
Copy link
Contributor Author

I would not agree on having public methods for "just in case..." and within this demo application it is not needed.
And I usually try to avoid those nullable properties if possible.

And no, you cannot combine readonly with setters. That's basically the feature :)

@javiereguiluz javiereguiluz merged commit c512f19 into symfony:main Oct 28, 2022
@javiereguiluz
Copy link
Member

Christopher, thanks for this nice contribution and sorry it took me so long to merge it.

Please note that while merging I tweaked the new Tag entity constructor a bit. See fbc76bb Your code is of course perfectly valid, but I think it could look too advanced/unconventional for some folks using this demo application. In the future we might change it again and use your version, but for now I'd prefer to use this more traditional constructor. Thanks!

@chr-hertel chr-hertel deleted the tag-remove-setname branch October 28, 2022 07:51
@chr-hertel
Copy link
Contributor Author

@javiereguiluz Totally get your point! :) Thanks for coming back to this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants